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/