Re: [boost] [http] Formal review of Boost.Http

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 
Hana.

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 
should Http.

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 
header only)

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 
production code.

https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook#a7.SAFETY:S
tronglyconsideranightlyorweeklyinputfuzzautomatedtestifyourlibraryisab
letoacceptuntrustedinput

Until you address a minimum of items 1 and 4, I am very sorry but I 
must vote for rejection.

Other notes:

* 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 
and Y?
  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 
outcomes.

* 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 
https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook. 

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 
since its 
inception, and I am looking forward to integrating a Http Filesystem 
backend 
into AFIO as soon as Http passes Boost review and gains a client side 
API.

Which, for the record, I believe it will. Your overall design is very 
solid, 
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 
problem.

Niall

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