[boost] [beast] Review

Subject: [boost] [beast] Review
From: Marcel Ebmer (marcel_at_[hidden])
Date: 2017-07-05 09:31:06

Here's my Review:

DESIGN
======
The design of concepts, data structures and algorithms is mostly done well, with
only minor concerns:
   - The smell of two-phase initialization.
     Required message properties like method and target have to be set from
     outside. This makes me as a library user afraid of forgetting something
     vital. The same holds for prepare_payload(). If I forget to call
     prepare_payload() or forget to set one of the required message properties or
     fields, my program may - in the worst case - appear to work correctly.
   - Inconsequent application of automagic.
     WebSocket control frames are automagically responded to. Iff there's an
     active read(). Basically, having an active read() at almost all times is a
     requirement of any correct program using Beast. Even though I enjoy the help
     from the library, and the requirement is usually met, I found this specific
     behaviour to be inconsequent.
     Also, the very first WebSocket example in the docs makes it clear that while
     a close from the peer is handled automagically under the above condition, a
     close from my side is quite tedious to write. There should be a utility in
     the library to do the draining for me.
   - The naming of BodyReader and BodyWriter initially got me confused.
     In my mind, I 'read' a body FROM a stream/buffer, I 'write' it INTO a stream
     or buffer. The library uses the terms in the reverse sense. This is not
     incorrect of course, but my initial confusion may be a sign of suboptimal
     naming.

IMPLEMENTATION
==============
Didn't look in-depth.

DOCUMENTATION
=============
The documentation is very well structured. It is complete in many, but not all
places.
   - A minimalistic HTTP server example would have helped me get started. Just
     something very, very basic. Reply to each request with "your request was".
   - In some places, non-self-explanatory function names suddenly pop up
     without being first introduced. For example, in the section
     "Using WebSocket"."Send and Receive Messages", text() and got_text() could
     use some explanation. In the same section, the reference to set_option()
     seems out-of-place and makes me wonder what I missed.
   - Since the documentation features comparison with other libraries, it may
     be worth comparing pion (https://github.com/splunk/pion) as well.
   - Many reference pages could use some prosaic explanation. For example, my
     uncertainty concerning message.prepare_payload() might have been alleviated,
     were it more clear what the function is intended for and what the effect of
     omitting it would be.

SCOPE
=====
Since the FAQ is quite defensive in regard to the library's scope, I feel this
is worth some discussion.
I found the scope to be just right for now, but with future potential.
As it is, the library takes away all the tedious protocol stuff, while leaving
all important design decisions to the user. It feels like an extension to ASIO.
However, there's no apparent reason why (e.g.) request targets shouldn't be
URL-decoded, and why form data should not be parsed into a map of some kind. The
way I see it, none of this added funtionality would interfere with the library's
design. It would, however, make the library user's life a whole lot easier, and
probably attract more users as well.

USEFULNESS
==========
See SCOPE. Very valuable, could be even better.

HOW MUCH EFFORT & TRIED TO USE
==============================
I invested about a day into the review, read the documentation and ported a
small REST-like service from pion to Beast. I may be biased towards Beast due to
temporal proximity, but I feel the latter made the code easier to reason about
and, at the very least, did not hurt performance. I used gcc 7.1 and clang 4.0.
On the review branch, none of the two had any issues.
I deliberately did NOT follow the discussion on the mailing list, in order to
remain as unbiased as practicable.

HOW KNOWLEDGABLE AM I?
======================
I have worked on several HTTP servers and experimented with writing an HTTP
server library a few years back. I have no practical experience with WebSockets.

IN ONE SENTENCE
===============
Beast should be ACCEPTED into Boost.

Marcel