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-08 15:06:14

I have seen enough reviews from others to be able to finish my review.
This is my formal review of the submitted Beast library.

> - What is your evaluation of the design?

It depends from where you are approaching.

As a niche Boost library of the kind most recent entrants to Boost are,
it's of good quality, solves a need, docs are good, testing is good,
implementation is not bad. One could argue it stops too far short for a
niche library, and it should fill in some of the obvious gaps as most
niche libraries do. But if accepted as so far most of the reviews
recommend, I think it'll fill out with time. It's not a concern for
acceptance.

As a fundamental essentials Boost library with potential for
standardisation (which the author has stated is the case, and I agree),
I found a lot of problems.

To clarify what I mean by "fundamental essentials", the really profound
thing about HTTP is that whenever you invent some mini request-reponse
protocol with tagged parameters and structured payload for some use
case, you inevitably end up reinventing HTTP. It's quite uncanny in
fact, almost disturbing, how often that wheel has been reinvented, and
usually very poorly. I have encountered so many lousy, low quality,
insecure, broken, memory corrupting, reinventions of HTTP in my career
it is quite depressing.

Hence there is a huge need for a properly written, fuzz tested,
low-level, lightweight, generalised request-response structured and
tagged data framework for C++ (and all languages) which is very widely
useful. Such a framework would have a very good chance of getting
standardised because it would be so damn useful for such a wide variety
of use cases, including traditional HTTP-over-socket, but also config
files, metadata, debug printing and so much more. Anything not so
general is correspondingly harder to standardise because I believe my
observations on this topic to be widely held and agreed with once people
think about it.

Why Beast falls far short of a fundamental essentials Boost library:

1. There is a lack of a well defined formal interface separating the
structured data parts from the i/o parts. This is never a good sign.

2. There is a lack of a well defined formal model abstracting away the
data parsing and generation model from a duplex stream model. Gavin said
a stream i/o model is a good one to target, I disagree. It is a
reasonable first but *wrong* guess to target, but in fact once you've
done a few of these you realise it is not optimal. Let me explain.

A generalised low-level request-response vocabulary library should not
impose a stream i/o model as there are so many widely used non-duplex
non-stream i/o models out there in use. In the past for example I've had
available to me a low bandwidth low latency transport and a high
bandwidth high latency transport. It makes huge sense to send the HTTP
headers via the low bandwidth low latency transport and the HTTP bodies
via the high bandwidth high latency transport. Another very common use
case is the ultra-high-latency transport better known as the file
system: how many times have people reinvented variations of
request-reponse protocol with tagged parameters and structured payload
now? INI, JSON, XML, TOML, YAML - they're all just a set of tagged
parameters and structured payload where request = read() and response =
write() over a non-duplex ultra high latency transport.

Those sorts of use case are easy implement with a generalised
"view/range of blobs of bytes" i/o model as I had proposed which is most
obviously implemented in C++ using a mix of span<T> and the Ranges TS.
It's much harder to implement, and with no corresponding gain either in
ease of use, with hard assumption of a duplex scatter-gather stream i/o
model. By hard coding the use of ASIO's inflexible and niche buffer
infrastructure, it makes Beast unusable as fundamental essentials
library, even if WebSocket, ASIO and all that other unnecessary stuff
were removed.

3. Minimum overhead design principles are not followed (zero copy, zero
malloc, zero blocking). The proposer will no doubt now aggressively
shout at me and harass me with "prove it in code". But I have given up
on trying to persuade him of anything, he makes it too unpleasant and mean.

Therefore as a fundamental essentials Boost library, Beast as proposed
is *not* generally useful because of how unnecessarily limited its use
cases are. If it were being reviewed as a fundamental essentials
library, it would have to be a REJECT with an encouragement that he's
done a good first attempt, but needs to think again.

> - What is your evaluation of the implementation?

The implementation is generally of high quality and I came away positive
from scanning the source code. Too much memory allocation, too much
memory copying for my taste, but it is entirely possible that that is
not important relative to the cost of implementing HTTP-over-socket anyway.

I had intended to run some comparative benchmarks between Beast and some
private code I wrote for clients here which uses all minimum overhead
design principles to see how much of a difference there might be. But
when the author made it clear he would not consider a departure from
being hardcoded to ASIO, I decided a further investment of my time here
would not be worth it, and it would require a fair few hours which are
better invested into Outcome v2 which is to be announced as feature
complete on Monday.

So all I can say is that the implementation is good enough for a niche
Boost library.

> - What is your evaluation of the documentation?

Pretty good. I got up to speed quickly. If the unnecessary parts of
Beast were removed and thus the documentation trimmed in size, it would
be better again.

> - What is your evaluation of the potential usefulness of the
> library?

No doubt it solves well its niche use cases.

> - Did you try to use the library? With which compiler(s)? Did you 
> have any problems?

I was going to, but decided not to bother given that the author would
not be receptive to any additional effort invested by me.

> - How much effort did you put into your evaluation? A glance? A
> quick reading? In-depth study?

About six hours. I did look at the library in detail back when
Boost.Http was being developed under GSoC, but I've not been paying much
attention since.

> - Are you knowledgeable about the problem domain?

Very. I have worked several contracts where a lightweight subset of HTTP
was implemented using all sorts of unusual non-socket transports. I have
been working on the ultimate low level foundation layer for all future
C++ i/o since 2012, and I have given very deep thought on how best to
implement minimum overhead i/o on very low latency hardware devices,
some of which has been discussed over several years with broad general
agreement as to my approach with various WG21 members, some very senior.

> Thank you for your effort in the Boost community.

Before I give my formal recommendation, I wish to preempt the inevitable
angry response to this review by telling the author that he did a good
library, but he's done a lousy review. One of the very hardest, most
valuable, rarest thing you can obtain in any knowledge based community
is high quality useful feedback on what's possible and desirable by
experts in their fields. That sort of high quality advice costs an
enormous sum of money to rent in, yet you can get it for free here if
you play your cards right. Had the proposer come here looking to make
the ultimate low level HTTP library, he could have come away with a
wealth of useful ideas of what's possible.

Instead he came here with a very narrow goal of getting his existing
library, tolerating only very minor changes, into Boost. And that's
okay, it's a fine niche library, but such a wasted opportunity on what
he could have achieved instead. But his loss in the end.

With all that being said, the final question to answer is whether Beast
is a niche Boost library or a fundamental essentials Boost library?

My own personal opinion (as is obvious above) sways towards fundamental
essentials. After all, it's supposed to be a *low-level* HTTP library.
But the wider community, based on the content of reviews to date, appear
to be thinking niche. I had a feeling that that would be the case, and
that's why I wanted to wait until enough reviews came in for me to be
able to call it.

Following the majority opinion that Beast is a niche not a fundamental
essentials Boost library, I therefore vote to ACCEPT the library without
conditions. It's a good library and a valuable addition to Boost. Well
done Vinnie!

Niall

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