Subject: Re: [boost] [http] Formal review of Boost.Http
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2015-08-07 11:53:15
On 7 Aug 2015 at 9:08, Bjorn Reese wrote:
> The source code is build using CMake, but Boost.Build is in the pipeline
> (already done for documentation.)
I'll give my frank opinion about Http, and note I had expressed this
privately by email some months ago but my opinion then was not heeded
before presenting this library for review. This has unfortunately
forced me to make these opinions public.
I do not believe Http is ready for peer review for these reasons, and
I personally would urge you withdraw it until you have fixed these
issues and then come back to us. If you do not withdraw it, I will
vote for rejection.
1. You require a C++ 11 compiler and are very much an extension of
ASIO throughout, and then don't take advantage of ASIO's extensive
internal C++ 11 vs Boost switching infrastructure. On the second page
of your documentation you state:
"std::error_code, for instance, cannot be transparently replaced by
boost::system::error_code within ASIO-using code"
This is utterly untrue, and I told you that several months ago. ASIO
itself maps some error_code implementation into namespace asio - go
look at its config.hpp. Therefore you simply use whatever error_code
ASIO itself uses, using the same internal STL abstraction
infrastructure ASIO uses. The same goes for every other instance
where you are using Boost dependencies.
I said several months ago that you should properly finish the ASIO
integration by ripping out all your current hard coded usage of Boost
and using ASIO's own STL abstraction layer, and thereby making Http
identical in supported platforms and configurations as ASIO (i.e.
standalone C++ 11 or with-Boost) reusing the same ASIO configuration
macros. I then suggested it would be a very short step to port Http
to C++ 03 as you are not using anything which particularly demands
C++ 11 outside the examples and tests and which ASIO hasn't
abstracted for you.
Http should be a model EXTENSION of ASIO in the cleanest possible
sense in my opinion. You're not far from achieving it, and I really
think you need to finish it.
2. You claim Boost.Build doesn't work outside the Boost tree. This is
untrue - Boost.Build doesn't need Boost. I don't mind cmake for the
unit testing alone as a temporary stopgap until you bite the
Boost.Build bullet before it enters Boost, but I draw the line at
requiring cmake for normal builds. Before presenting for review
normal library builds cannot use cmake OR should be header only like
3. ... which leads me to asking why is Http not a header only library
like ASIO? That would eliminate your Boost.Build requirement in the
strictest sense. Standalone ASIO doesn't need Boost.Build. Neither
You mention in the FAQ the reason why not header only is mainly due
to the use of a third party HTTP C parser. It could be insufficient
familiarity on my part with the vaguaries of parsing HTTP, but it has
never struck me as difficult to parse - you don't even need to
tokenise, and indeed my first instinct would be it is better to NOT
tokenise HTTP as that is both slower and opens a wider attack surface
to a hand written "stupid" parser.
(BTW static custom error code categories work just fine header only
if correctly set up. Indeed Boost.System can be configured to be
4. Http is the first line of parsing code in a world full of
malicious exploitation - just this week all Android devices were made
powned with a single malformed MMS message causing a buffer overflow.
Anything parsing untrusted input needs to be regularly input fuzz
tested period, and very especially Http. Until Http is running at
least one of the below fuzz tools (american fuzzy lop or LLVM
libfuzzer) at least weekly on a CI it is not safe to allow into
Until you address a minimum of items 1 and 4, I am very sorry but I
must vote for rejection.
* I don't understand why you cannot issue more than one async read at
a time nor async write at a time. In something like pipelined HTTP
that seems like a very common pattern. I smell a potential design
flaw, and while the docs mention a "queue_socket" I can't see such a
thing in the reference section.
* Your tutorial examples use "using namespace std;" at global level.
That's a showstopper of bad practice, and you need to purge all of
those. Same for "using namespace boost;" at global level.
* Your reference section is one very long thin list. You might want
to use more horizontal space.
* I personally found the design rationale not useful. I specifically
want to know:
1. Why did Http choose ASIO as my base over leading competitor X
2. What sacrifices occurred due to that choice? i.e. things I'd
love weren't the case.
3. What alternatives may be better suited for the user examining
Http for suitability and why? i.e. make your documentation useful to
people with a problem to solve, not just a howto manual.
* Benchmarks, especially latency ones which are important for many
uses of HTTP are missing. I'd particularly like to know confidence
intervals on the statistical distribution so I know worst case
* Personally speaking, I'd like if in your tutorial you took ten
self-contained steps in building a simple HTTP fileserver, each with
a finished working program at the end of each section. Like the ASIO
tutorial does. In particular, I'd like to see layers which add on
etags and other of the fancy facilities in stages.
* I would personally worry about investing on developing code based
on Http until you have HTTP 2.0 support in there, only because I
would suspect you might need to break the API a bit to get the
all-binary format of HTTP 2.0 working as it's such a change from HTTP
1.0. It may merely be paranoia, but I suspect I wouldn't be alone on
this silent assumption.
* You're still missing CI testing with valgrind, thread sanitiser,
coveralls.io coverage testing, etc etc all the stuff from
I hope all the above isn't too disheartening VinÃcius. If you took
six weeks extra to finish the port of Http to ASIO proper, added a
Jamfile for the main library build, and patched in some input fuzz
testing (I don't mind if the tests fail across the board BTW, I just
want to see the HTTP parser fuzz tested regularly) I would have no
objection to Http being reviewed and I believe you would get a
conditional acceptance from me at least. As Http currently stands, I
don't believe it is currently ready for review.
I should also add that I have been following this project intently
inception, and I am looking forward to integrating a Http Filesystem
into AFIO as soon as Http passes Boost review and gains a client side
Which, for the record, I believe it will. Your overall design is very
you had an excellent mentor during GSoC, all you need is (quite a
lot) more polish. I don't find any fundamental design mistakes in
Http apart from the bad design smell surrounding lack of concurrency
in async read and write, and even that could just be a documentation
-- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk