Boost logo

Boost :

Date view Thread view Subject view Author view

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-12-04 23:43:48

On 12/5/23 02:13, Andrzej Krzemienski wrote:
>
>
> pon., 4 gru 2023 o 00:29 Andrey Semashev via Boost
> <boost_at_[hidden] <mailto:boost_at_[hidden]>> napisał(a):
>
> On 12/3/23 19:26, Andrzej Krzemienski wrote:
> > niedz., 3 gru 2023 o 15:56 Andrey Semashev via Boost
> > <boost_at_[hidden] <mailto:boost_at_[hidden]>
> <mailto:boost_at_[hidden] <mailto:boost_at_[hidden]>>>
> napisał(a):
> >
> >     > Why do we have scope_success? An alternative to using scope
> >     success is to
> >     > just put your instructions at the end of the function. Do
> you have
> >     multiple
> >     > return statements in your function and at the same time you need
> >     the same
> >     > sequence of operations to be performed at each end? This is
> a good
> >     > motivation to restructure your function to have a single return.
> >     Multiple
> >     > returns are generally OK, but needing the same sequence
> warrants a
> >     single
> >     > return.
> >
> >     What if having a single return is difficult to achieve?
> Sprinkling `goto
> >     finish` doesn't work well sometimes, e.g. when you have variable
> >     definitions in the middle of the function. It is also more
> prone to
> >     errors.
> >
> > Further discussion would be easier if you were able to provide an
> > example of such a function.
> > It may be just my lacking imagination, but in my experience any
> > complicated function could be
> > refactored, e.g. some parts split to separate functions or lambdas, so
> > that a single return could be achieved, or multiple returns without
> > identical sequence of operations.
> > A good example could prove me wrong.
>
> Ok, here's a candidate that could use a scope_success. The method
> handles an incoming RTSP request. It is supposed to analyze the request
> validity, parse its common headers, find a session which the request
> belongs to and invoke a handler in that session. In any case, the method
> is supposed to send a response to that request - either a positive 200
> or an error. Naturally, depending on the outcome, there may be different
> additional headers that need to be added to the response.
>
> Extremely simplified, it currently looks something like this:
>
>   void receiver::on_request(request const& req)
>   {
>     response resp;
>
>     {
>       auto it = req.headers().find(cseq);
>       if (req.start_line().version == protocol_version::unknown)
>       {
>         resp.start_line().version = m_protocol_version;
>         resp.start_line().set_status(version_not_supported);
>
>         if (it != req.headers().end())
>           resp.headers().set(cseq, it->value(), it->is_sensitive());
>
>         resp.set_body(text_plain, rtsp_version_not_supported_body);
>
>         goto send_response;
>       }
>
>       resp.start_line().version = req.start_line().version;
>
>       if (it == req.headers().end())
>       {
>         resp.start_line().set_status(bad_request);
>         goto send_response;
>       }
>
>       // ... and so on, continue to validate the request, filling
>       // the response as needed, based on the request, and jumping
>       // to send_response below in case of errors
>     }
>
>     {
>       rtsp_session* rtsp_sess = find_session(req);
>       if (!rtsp_sess)
>       {
>         resp.start_line().set_status(session_not_found);
>         goto send_response;
>       }
>
>       switch (req.start_line().method)
>       {
>         // For each supported method, analyze request headers
>         // that pertain to that method, invoke a handler in the
>         // session or otherwise handle it. Then either break or
>         // goto send_response.
>       }
>
>       // At this point the request is handled successfully
>       resp.start_line().set_status(ok);
>     }
>
>   send_response:
>     send_message(resp, [this](error_code const& error)
>     {
>       // Handle sending errors, if any
>     });
>   }
>
> You can see the pattern here. resp gets populated according to the
> request and the processing results, then it gets sent in the end.
>
> This code could be simplified by wrapping the send_message call in a
> scope_success at the beginning of the function. This would remove the
> need to introduce additional scopes to make the goto work, and would
> allow to replace all gotos with returns. I could also accompany it with
> a scope_fail to send a special response in case of exception.
> (Currently, exceptions are handled elsewhere.)
>
>   void receiver::on_request(request const& req)
>   {
>     response resp;
>
>     scope_success success_guard{[&]
>     {
>       send_message(resp, [this](error_code const& error)
>       {
>         // Handle sending errors, if any
>       });
>     }};
>
>     scope_fail failure_guard{[&]
>     {
>       send_fatal_response_and_disconnect(resp); // doesn't throw
>     }};
>
>     auto it = req.headers().find(cseq);
>     if (req.start_line().version == protocol_version::unknown)
>     {
>       // ...
>       return;
>     }
>
>     // etc.
>   }
>
> Also, separate from the example above, a few other use cases. Although
> not quite the same as scope_success, it appears I'm using a similar
> concept in Boost.Log in a couple of places:
>
> https://github.com/boostorg/log/blob/8b921b6352e76ff54f5674c864d1f4f749c9158b/include/boost/log/sources/record_ostream.hpp#L460-L539 <https://github.com/boostorg/log/blob/8b921b6352e76ff54f5674c864d1f4f749c9158b/include/boost/log/sources/record_ostream.hpp#L460-L539>
>
> https://github.com/boostorg/log/blob/8b921b6352e76ff54f5674c864d1f4f749c9158b/include/boost/log/detail/format.hpp#L246-L331 <https://github.com/boostorg/log/blob/8b921b6352e76ff54f5674c864d1f4f749c9158b/include/boost/log/detail/format.hpp#L246-L331>
>
> In both cases, the classes work a "pumps". I.e. they initiate data
> collection on construction, then the user provides the data (by calling
> operator<< or operator% to format the log message), then on destruction,
> if no exception was thrown, the pump pushes the collected data to the
> logger. So, for example, in the following expression:
>
>   BOOST_LOG(lg) << "Hello " << "world!";
>
> BOOST_LOG macro creates the record_pump object, then the operator<<
> calls compose the log message, then the record_pump object is destroyed
> and, if the operator<< calls didn't throw, pushes the log record into
> the logger. If an exception is thrown then the message is discarded.
>
> This design is influenced by the required syntax - I need to push the
> record *after* the message is formatted. But I believe, such a pattern
> is not uncommon and could use scope_success for this purpose. In a way,
> the previous example with RTSP request handling also fits this pattern -
> you create a scope guard, then you populate some data structure, then,
> if everything is ok, you send that data structure for further
> processing.
>
> I will also note that there are a few examples in D documentation
> regarding scope guards:
>
> https://dlang.org/articles/exception-safe.html
> <https://dlang.org/articles/exception-safe.html>
>
>
> Thanks. 
>
> The "before" and "after" samples seem to do a different thing (the
> "after" also displays the failure message).

Um, no, where did you see that? The only difference is the added
failure_guard.

> But I thing the "before
> example could be refactored to:
>
>   void receiver::on_request(request const& req)
>   {
>     response resp = [&]  // IILF
>     {

Presumably, you also need to construct resp here.

>       auto it = req.headers().find(cseq);
>       if (req.start_line().version == protocol_version::unknown)
>       {
>         resp.start_line().version = m_protocol_version;
>         resp.start_line().set_status(version_not_supported);
>
>         if (it != req.headers().end())
>           resp.headers().set(cseq, it->value(), it->is_sensitive());
>
>         resp.set_body(text_plain, rtsp_version_not_supported_body);
>
>         return resp;
>       }
>
>       resp.start_line().version = req.start_line().version;
>
>       if (it == req.headers().end())
>       {
>         resp.start_line().set_status(bad_request);
>         return resp;
>       }
>
>       // ... and so on, continue to validate the request, filling
>       // the response as needed, based on the request, and jumping
>       // to send_response below in case of errors
>    
>       rtsp_session* rtsp_sess = find_session(req);
>       if (!rtsp_sess)
>       {
>         resp.start_line().set_status(session_not_found);
>         return resp;
>       }
>
>       switch (req.start_line().method)
>       {
>         //
>       }
>
>       // At this point the request is handled successfully
>       resp.start_line().set_status(ok);
>      
>       return resp;
>     }();
>
>     send_message(resp, [this](error_code const& error)
>     {
>       // Handle sending errors, if any
>     });
>   } 

That would add an extra copy or move of response to return from the
lambda, but yes, that could work. I guess, it's a matter of how easy it
is to perform this transformation and how much overhead it would be. And
also a personal preference.

> Aldo, one observation regarding this piece of code:
>
>     scope_success success_guard{[&]
>     {
>       send_message(resp, [this](error_code const& error)
>       {
>         // Handle sending errors, if any
>       });
>     }};
>
>     scope_fail failure_guard{[&]
>     {
>       send_fatal_response_and_disconnect(resp); // doesn't throw
>     }};
>
> This use case is not served best by two scope guards, as each of them
> incurs the same cost of exception checking. Maybe it would be served
> better by a yet another guard type that takes two callbacks: one
> representing the action on success, the other the action on failure.

Yes, the exception would be checked twice, but I don't expect this to be
expensive. The idea of a scope guard with two callbacks also occurred to
me, but it would probably be better to do something like this instead:

  BOOST_SCOPE_FINAL [&, check_ex = exception_checker()]
  {
    if (!check_ex())
    {
      // No exception
    }
    else
    {
      // Exception was thrown
    }
  };

> One other, unrelated, observation. When I need to make sure that three
> things happen together or none does: the most efficient way to do it
> seems to be:
>
> void update_three(Compo const& val)
> {
>   bool armed = true;
>
>   vec1.push_back(val);
>   BOOST_SCOPE_FINAL [&] { if (armed) vec1.pop_back(); };
>
>   vec2.push_back(val);
>   BOOST_SCOPE_FINAL [&] { if (armed) vec2.pop_back(); };
>
>   vec3.push_back(val);
>   armed = false;
> }

I would use scope_fail instead of both BOOST_SCOPE_FINAL and `armed`.

Date view Thread view Subject view Author view

Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk