Re: [boost] [beast] Formal review

Subject: Re: [boost] [beast] Formal review
From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2017-07-12 22:03:45

On Sun, Jul 9, 2017 at 1:57 PM, Bjorn Reese via Boost
<boost_at_[hidden]> wrote:
> There are too many goto statements for my taste;I do not mind the goto
> pattern used in the parser, but the state-machines in read_some_op and
> serializer looks more like micro-optimization.

That's purely your opinion and not a very insightful one at that
because break and goto are efffectively the same thing, except that in
beast's more complex composed operation state machines:

    break;

indicates that the state machine will run for another loop (transition
to a new state), while

    goto upcall;

is used to annotate that the composed operation is terminating. All
composed operations must 1. eventually call the final handler, and
2.deallocate all temporary allocations before invoking the final
handler (the "deallocation-before-invocation" guarantee).

The rules for composed operations are strict, and the code is written
in a style that minimizes the possibility of errors by expressing
intent clearly. The subject is important enough that the documentation
includes an entire section of helpful tips on how to avoid common
mistakes when writing composed operations (bottom of page):

<http://vinniefalco.github.io/stage/beast/review/beast/using_networking/writing_composed_operations.html>

> Furthermore, "case do_body + 1:" looks like an awful hack.

Again subjective and indicative more of a less than thorough
understanding of the implementation. Named case labels denote
"subroutines" within the state machine while increments represent line
numbers. This keeps the code organized and makes things easier when
making changes,

Quite the opposite of an awful hack, beast websocket composed
operations have *intense*, comprehensive unit tests. They use
coroutines and manually advance the io_service one completion at a
time to set up the state of a client/server pair in order to exercise
every code path. That code is here:

<https://github.com/vinniefalco/Beast/blob/review/test/websocket/stream.cpp#L1325>

Beast websocket has a very special feature, it allows a pending
asynchronous read to auto-respond to pings and close frames (which
perform socket writes), even while an asynchronous write is pending.
It also allows an asynchronous ping to be sent even while both an
asynchronous write and asynchronous read are pending! This is no
trivial implementation (pending composed operations are "parked" in a
separate object and resumed later). Its all very well tested with
great coverage. Does this coverage look like "awful hack?"

<https://codecov.io/gh/vinniefalco/Beast/src/review/include/beast/websocket/impl/read.ipp#L152.

The mischaracterization of Beast code is probably an honest mistake on
your part.

Thanks