[boost] [beast] Review

Subject: [boost] [beast] Review
From: Peter Dimov (lists_at_[hidden])
Date: 2017-07-03 23:23:38

This is my review of the Beast library.

I. DESIGN
---------

The design of the library is very good and appears mature. The concepts are 
well specified, well documented, well thought out. The use of concept checks 
is a nice touch and a mark of professionalism.

I think that basing the library on ASIO (resp. the Networking TS) is a fine 
decision. This does mean that if one finds an ASIO/NetTS concept less than 
ideal, one has to live with it, but that's an acceptable tradeoff.

Severing the ties with ASIO based on the yet-hypothetical "structured blobs 
in, structured blobs out" model may sound good in theory, but in practice, a 
supermajority of the potential users of Beast need to produce a working HTTP 
or Websocket server, and the library as it stands addresses this need.

The design is not perfect; at times the library makes it much too easy for 
asymptomatic mistakes to be introduced by the omission of a required member 
function call. For example, if one forgets to set the Content-Length, 
everything will still appear to work; or if one forgets to initialize the 
.result of the response, it remains uninitialized. (Note that these, and 
subsequent, observations appertain to the library as submitted for review; 
they may no longer be relevant since the author is very quick with the 
fixes.)

There is, at times, unnecessary verbosity, even in the two storefront 
examples in

http://vinniefalco.github.io/stage/beast/review/beast/quick_start.html

that are supposed to show the library in the best possible light. For 
instance,

    // Set up an HTTP GET request message
    http::request<http::string_body> req;
    req.method(http::verb::get);
    req.target("/");
    req.version = 11;

Every request will need a method and a target, so having to call members to 
set them feels unnecessary. A constructor ought to have been provided.

Similarly, in the next example,

    // WebSocket says that to close a connection you have
    // to keep reading messages until you receive a close frame.
    // Beast delivers the close frame as an error from read.
    //
    beast::drain_buffer drain; // Throws everything away efficiently
    for(;;)
    {
        // Keep reading messages...
        ws.read(drain, ec);

        // ...until we get the special error code
        if(ec == websocket::error::closed)
            break;

        // Some other error occurred, report it and exit.
        if(ec)
            return fail("close", ec);
    }

the drain loop is basically a requirement for every correct program, so it 
should have been encapsulated into a helper function. (Even though the 
example as written does illustrate the use of a drain_buffer.)

I'm not particularly fond of the design decision to declare parts that in 
practice every user of the library will need as "out of scope". People 
should not be made to reinvent these wheels. I understand the aim of 
producing a standard library proposal, and that some of these wheels would 
not be suitable for standardization, but there's nothing easier than just 
including the suitable wheels in the formal proposal and omitting the 
unsuitable ones.

In practice, these necessary parts end up as examples, so one has to 
#include "../example/wheel.hpp" instead of "beast/wheel.hpp". It would be 
better, in my opinion, if one includes, say, "beast/ext/wheel.hpp", with it 
being known that ext/ is where "out of scope" things go.

The ASIO coupling did leave me with one question, whether it would be 
possible for the library to accommodate OS-specific file to socket transfers 
such as the one provided by TransmitFile:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms740565(v=vs.85).aspx

The idea here is that the OS kernel can dump the file to the socket without 
involving user mode, which has the potential of much increased performance.

For the median user, this would not make much of a difference, but it's an 
interesting design question, and perhaps an argument that the example 
file_body class ought to be part of the library proper. ("Is in scope", to 
use the preferred terminology.)

II. IMPLEMENTATION
------------------

I did not pay close attention to the implementation. A random sampling 
reveals that it's professionally written, and it appears to work, which is 
good enough for me. The tests run when `b2` is invoked in `test`, have good 
coverage, and pass with MSVC 14.0 and 14.1.

Travis/Appveyor are put to good use.

The library provides a doc/Jamfile, which is a plus, but this Jamfile fails, 
which is a minus. The file reference.qbk has its own separate build script, 
and this would need to be fixed one way or another when the library is 
integrated into the Boost documentation build.

III. DOCUMENTATION
------------------

The documentation is good. The non-reference parts are very good, the 
examples are informative. The reference is very good at times, and not so 
much at others. This is caused by the fact that to get to the good parts one 
needs to drill down a level or two. For instance, looking at

http://vinniefalco.github.io/stage/beast/review/beast/quickref.html

I noticed the "async_teardown" function in the WebSocket section, so I 
clicked on it to see if I need to call that to tear down a WebSocket 
connection. This lead me to

http://vinniefalco.github.io/stage/beast/review/beast/ref/beast__websocket__async_teardown.html

which only says "Start tearing down a 
boost::asio::ssl::stream/connection/boost::asio::ip::tcp::socket."

Googling told me that I'm not the only one left not entirely satisfied with 
these descriptions:

https://github.com/vinniefalco/Beast/issues/274

The confusion here stems from the fact that would I, and the issue 
submitter, have clicked on one of the async_teardowns in that page, we would 
have reached f.ex.

http://vinniefalco.github.io/stage/beast/review/beast/ref/beast__websocket__async_teardown/overload1.html

which is indeed fine, and mentions that the implementation calls this 
function, not the user.

This is a general pattern with the reference. The leaves are perfectly fine, 
but the upper levels are often uninformative.

IV. PRACTICAL USE
-----------------

As part of conducting this review, I ported two existing servers of mine to 
Beast, one HTTP, one WebSocket. The HTTP server can be seen at

https://github.com/pdimov/beast_tpc_server

and the WebSocket one is at

https://github.com/pdimov/beast_ws_async_server

The experience was pleasant and things went more or less as expected, with 
virtually no nasty surprises. The author was very helpful. The library 
proper worked exactly as advertised, and the servers were both easier to 
write than the originals, and became more robust.

I only wish that more out of scope things (target parser, basic 
authorization handling) become in-scope, because their absence is being 
felt.

V. VERDICT
----------

Beast should be ACCEPTED. It's useful, it works, it serves a legitimate 
need, and I see no design or implementation problems that would preclude 
acceptance.