Re: [scope] some more thoughts

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2023-12-04 23:13:14

pon., 4 gru 2023 o 00:29 Andrey Semashev via Boost <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]>> 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/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
>
>
Thanks.

The "before" and "after" samples seem to do a different thing (the "after"
also displays the failure message). But I thing the "before example could
be refactored to:

  void receiver::on_request(request const& req)
  {
    response resp = [&]  // IILF
    {
      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
    });
  }

The D documentation examples seem artificial.I think yours is better.

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.

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;
}

Regards,
&rzej;