Re: [boost] [beast] Formal review

Subject: Re: [boost] [beast] Formal review
From: Edward Diener (eldiener_at_[hidden])
Date: 2017-07-13 00:40:24

On 7/12/2017 6:03 PM, Vinnie Falco via Boost wrote:
> 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,

Boost has two state machine libraries. Did you look at either of them 
for your uses ?

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