Re: [boost] [beast] Formal review

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 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/writi...

...

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#L...

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/i....

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

Thanks