Re: [boost] [review][beast] Review of Beast starts today : July 1 - July 10

Subject: Re: [boost] [review][beast] Review of Beast starts today : July 1 - July 10
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2017-07-01 14:46:02

> Please consider participating in this review. The success of Boost is
> partially a result of the quality review process which is conducted by
> the community. You are part of the Boost community. I will be grateful
> to receive a review based on whatever level of effort or time you can
> devote.

As this is such a big library, some early first impressions/notes.
Positives:

+ This library is far better quality than when I last looked at it which
was actually probably a long time ago. Congrats to Vinnie on that. It
ain't easy getting it to this far.

+ I pretty much agree with everything in his design rationales in the
section at
https://vinniefalco.github.io/stage/beast/review/beast/design_choices.html
where he compares to other HTTP C++ implementations.

+ I found the docs clear, about the right balance between detail and
simplicity, and I understood everything I felt I needed to know quickly.
Well done on that as well Vinnie, it's a hard balance to achieve.

+ I very much agree with the choices in boundary separating what to
provide and what not to provide, with only a few niggles, but nothing
major and nothing showstopper. I feel a lot of the criticism seen to
date about that choice is meritless once you actually understand real
world implementations of HTTP.

Negatives:

- For this collection of HTTP-related library routines which is what
this library actually is, I do not see why any awareness of ASIO is
needed at all. I don't get why it's needed, and moreover, I think the
library would be far better off removing all awareness of a networking
implementation entirely.

If Beast implemented full HTTP like Boost.Http tried to do, I could see
the value add in tying in tightly to some networking implementation or
other. But it does not implement HTTP, it implements a set of routines
that one would use to implement HTTP instead. Most of the routines Beast
supplies that I examined would be more reusable, easier to use, and less
templatey if they made zero assumptions about what networking
implementation is in use.

- Everything appears to be a template. This sort of API design is forced
on Beast by the tight dependency on ASIO whose public API is almost
entirely templated. I could see that this design choice of ASIO's made
sense in C++ 98 days with the knowledge of best practices then extant in
2005, but in a C++ 11 library designed after 2015 it is the wrong design
choice. We simply know better nowadays, plus WG21 has much superior
vocabulary types for doing buffer sequences than ASIO's which are
needlessly complex, over engineered, and over wraught. A new library
should not repeat the design mistakes of ASIO unless it has to, and I
see no compelling reason given the limited implementation of HTTP
offered by Beast to have any dependency on ASIO at all.

Beast does a great job of drawing the correct boundary around what of
HTTP to implement and what not to implement. I very much agree with that
boundary. But the choice to model ASIO was made superfluous by that
boundary choice. The ASIO design model should be replaced with a more
generic reusable design instead, one which works with any arbitrary
socket API - or indeed, non-socket API.

An an example of a non-socket API, imagine using the UDT stream layer
instead of TCP (http://udt.sourceforge.net/) and implementing HTTP on
top of that. UDT provides its own library implementation and API which
somewhat looks like a socket API, but varies in significant ways.

With Beast's current design, trying to use Beast with the UDT API is
going to be unnatural and awkward. If Beast were instead a reusable,
generic, HTTP related utility library, it would be a much better library
for it.

- The choice to use STL allocators I feel is a code smell. I think a
correct HTTP primitive library would never allocate memory, and thus
never need to call a STL allocator. I think all the places where Beast
does allocate memory were driven by the hard dependency on ASIO, and if
you removed ASIO entirely, you would remove any need for memory allocation.

- I think basic_parser should be designed to use forward iterators
instead of requiring random access, or accept partial input in chunks
whilst keeping state. This would allow it to work seamlessly with
discontiguous underlying storage.

- I appreciate that this my final negative will not be widely agreed
with on boost-dev, but personally speaking I think Beast should work
well with C++ exceptions disabled, and therefore make no use of
exception throws at all. There are lots of end users who would like to
speak HTTP without enabling C++ exceptions for cultural or safety
validation reasons: embedded devices, IoT, games, medical devices etc.
Beast is not useful to any of those use cases if it insists on ASIO and
throws exceptions, neither of which are necessary nor make for a better
HTTP utilities library.

So to sum up, I like the library, most of the design choices are
sensible, I think it would save me a lot of hassle when implementing
HTTP as-is. But I feel it is arse-about-face in design: a simpler, less
cluttered, no-ASIO, no-malloc, less-templates design would be much
better again.

I'm not going to issue a final review now. I specifically want to see
what some others say first, most specifically Bjorn Reese who knows this
area very, very deeply and indeed mentored Boost.Http over multiple
summers. He likely will have a different weighting of priorities to me,
and he's far more a domain expert in this than I am, so his review will
influence mine.

I would also be interested in seeing the proposer's opinion on
everything I've just said before I issue a formal review. I do want to
close with saying that it's a great library Vinnie, you did a great job.
But I think a different design focus would be a lot better again for its
given scope and remit.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/