[http] Formal review of Boost.Http
Dear Boost and ASIO communities. The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th. Boost.Http is designed for various forms of modern HTTP interaction, from normal HTTP request, over HTTP chunking and pipelining, to upgrading to other web protocols like WebSocket. This library builds on top of Boost.ASIO, and follows the threading model of ASIO. The two basic building-blocks are http::socket, which is socket that talks HTTP, and http::message with contains HTTP meta-data and body information. You can use these building-blocks to build a HTTP server that fits your exact needs; for instance, an embedded HTTP server for a ReST API. Boost.Http comes with a light-weight HTTP server and a static file server. Currently, Boost.Http is limited to server-side interaction, but the design principles used extends to client-side as well. Boost.Http was originally developed as part of GSoC 2014 and Vinícius has continued to develop and improve the library since then. The documentation can be found here: http://boostgsoc14.github.io/boost.http/ The source code can be found here: https://github.com/BoostGSoC14/boost.http The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.) Please answer the following questions: 1. Should Boost.Http be accepted into Boost? Please state all conditions for acceptance explicity. 2. What is your evaluation of the design? 3. What is your evaluation of the implementation? 4. What is your evaluation of the documentation? 5. What is your evaluation of the potential usefulness of the library? 6. Did you try to use the library? With what compiler? Did you have any problems? 7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? 8. Are you knowledgeable about the problem domain?
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/
Hi Vinícius, I noticed your roadmap includes benchmarks. Do you have any preliminary benchmarks that you could share during the review period? Perhaps to: - cpp-netlib - poco - casablanca - proxygen Best, Glen -- View this message in context: http://boost.2283326.n4.nabble.com/http-Formal-review-of-Boost-Http-tp467860... Sent from the Boost - Dev mailing list archive at Nabble.com.
2015-08-07 15:24 GMT-03:00 Glen Fernandes
Do you have any preliminary benchmarks that you could share during the review period?
Perhaps to: - cpp-netlib - poco - casablanca - proxygen
I could provide a sloopy benchmark (just static hello world) during this weekend, but I don't think it'd be too representative (not near too a real-world application). I use the Node.js's HTTP parser, so performance should be Node.js without all the JavaScript engine stuff (and the possibility of multi-threading). And I'm writing a reply to Niall's email, but his email is so big that I don't think I'll finish the reply today. Sorry, Niall (and thanks for the effort you've put into your feedback). T_T -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
2015-08-07 15:41 GMT-03:00 Vinícius dos Santos Oliveira < vini.ipsmaker@gmail.com>:
2015-08-07 15:24 GMT-03:00 Glen Fernandes
: Do you have any preliminary benchmarks that you could share during the review period?
Perhaps to: - cpp-netlib - poco - casablanca - proxygen
I could provide a sloopy benchmark (just static hello world) during this weekend
Application: get the host header, get the request uri, concatenate both and send as reply. Tested using weighttp -n 20000 -c 1000 -k http://localhost:8080/ https://gist.github.com/vinipsmaker/c2455df44e53bb305ca1 You can see that performance of Boost.Http versus Pion was roughly the same, but Boost.Http created no extra threads. If I used one extra thread, I think I could see a speedup. Also, notice that the pion example didn't log any output to the standard output, while the Boost.Http is printing everything that happens. There is also the POCO test. I'd say the performance is promising. -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
2015-08-09 23:38 GMT+08:00 Vinícius dos Santos Oliveira < vini.ipsmaker@gmail.com>:
2015-08-07 15:41 GMT-03:00 Vinícius dos Santos Oliveira < vini.ipsmaker@gmail.com>:
2015-08-07 15:24 GMT-03:00 Glen Fernandes
: Do you have any preliminary benchmarks that you could share during the review period?
Perhaps to: - cpp-netlib - poco - casablanca - proxygen
I could provide a sloopy benchmark (just static hello world) during this weekend
Application: get the host header, get the request uri, concatenate both and send as reply.
Tested using weighttp -n 20000 -c 1000 -k http://localhost:8080/
https://gist.github.com/vinipsmaker/c2455df44e53bb305ca1
You can see that performance of Boost.Http versus Pion was roughly the same, but Boost.Http created no extra threads. If I used one extra thread, I think I could see a speedup. Also, notice that the pion example didn't log any output to the standard output, while the Boost.Http is printing everything that happens.
Why spawning a coroutine at L200 while you don't actually need it?
2015-08-09 13:19 GMT-03:00 TONGARI J
Why spawning a coroutine at L200 while you don't actually need it?
Because I was lazy to remove the coroutine at that point and I was too confident that Boost.Http still would show itself competitive with that coroutine (and my feeling was right). -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
Hi Vinícius, You wrote:
You can see that performance of Boost.Http versus Pion was roughly the same, but Boost.Http created no extra threads. If I used one extra thread, I think I could see a speedup. Also, notice that the pion example didn't log any output to the standard output, while the Boost.Http is printing everything that happens.
Thank you for the first benchmarks. I notice the Poco one is still in progress. The benchmarks so far (and any more that you do) are very interesting to me. Besides a nice way to show me that Boost.Http can be used to implement services that perform better (or no worse) than other libraries, it also shows me how easy it is to implement the same simple service with all these libraries. I am curious about the fact that different HTTP server libraries use different approaches on Windows. I noticed that Casablanca, while having an ASIO based implementation for portability, also has a HTTP.SYS based implementation for NT. It would be pretty neat if it turns out that Boost.Http on Windows performs equally (or really neat if it performs better) than using the HTTP server APIs provided with the OS. Especially when there are now other, non-performance, reasons to avoid the HTTP server APIs. The details I would find interesting to see per benchmark (columns per library you are comparing): - Attempted/connected - Memory used (kilobytes) - Non-paged pool - CPU usage - Threads - Throughput (send/receive bytes per second) Casablanca, poco, cpp-netlib, proxygen and (as you've shown me in your example code) pion provide high level HTTP abstractions. It will be nice for me to see what your Boost.Http benchmark code looks like when it uses the high level API you plan to write. Best, Glen -- View this message in context: http://boost.2283326.n4.nabble.com/http-Formal-review-of-Boost-Http-tp467860... Sent from the Boost - Dev mailing list archive at Nabble.com.
2015-08-07 12:53 GMT-03:00 Niall Douglas
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.
Can I persuade you to review the Boost.Http design? I'd reject myself if you find any fundamental flaw on the design and even if you're going to reject the library because the lack of tooling (not directly related to design or documentation, which are the most important items on any library), it'd be useful to know what needs to be addressed on the design before I submit again (in the case the submission is rejected). 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.
It's not my intention to make Boost.Http work with C++. I use Boost as dependency and I'm not willing to change that (unless the pieces I use from Boost enter on the C++ standard). About C++03 support, it's not required for Boost acceptance, so I can add later. I want to increase tests coverage a lot before porting code to C++03, so I don't introduce accidental bugs in the process. In the case the abstractions I need enter on the C++ standard, maybe your APIbind will help me. 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 tried to find such macros on Asio and I couldn't find any for error_code: http://www.boost.org/doc/libs/1_58_0/doc/html/boost_asio/overview/cpp2011.ht... Now that you mentioned config.hpp, I used my grep/find skills and I found: BOOST_ASIO_HAS_STD_SYSTEM_ERROR But I just don't find any reference to BOOST_ASIO_HAS_STD_SYSTEM_ERROR outside of config.hpp. Even if it was used, it is undocumented, which means it is an implementation detail that won't be guaranteed to be there forever (accidental behaviour versus a documented guarantee). 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.
Delay acceptance of the library will only delay its usage. Design of core abstractions is finished and I fail to see why its acceptance should be delayed while I improve implementation details. Standalone Boost.Http isn't as exciting as a WebSocket implementation. There is a roadmap of the features: https://boostgsoc14.github.io/boost.http/design_choices/roadmap.html C++03 is on the roadmap, but standalone Boost.Http is not. 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.
CMake will be replaced by Boost.Build before any integration. 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)
This parser is well tested and largely used. I want to increase test coverage a lot before replacing the parser with one of my own. HTTP spec used to be very confusing before RFC7230. Just now it's easy to implement all corner-cases. Previously, you needed a lot of experience. 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
That's why this first implementation uses a proven-parser that is very well tested. All tests from the Boost.Http socket will run with varying buffer sizes (from 1 to 2047). Of course I do enjoy tests and I'm constantly implementing more (the last experiment was using sanitizers). 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.
Too much synchronization and buffering. The pipeline has the following requests: A => B => C If I were to allow replies to request B, the design would be more complex, increasing the difficult to implement alternative backends and everyone's life. Also, the reply to request B cannot actually be sent while the reply to request A is on hold. This means that the whole response to request B would need buffering and if response B is a video live stream, this means an implicit (the user cannot know) crash after an unfortunate memory exhaustion. I couldn't claim a **lightweight** server if this excessive buffering was to occur. I couldn't claim the project is appropriate for embedded devices if I had chosen the buffering approach. It's a trade-off. I should write about it in the "design choice" of the documentation. HTTP/2.0 does have proper multiplexing and it can be more useful to respond several requests without "head of line blocking". A queue socket is a concept, I don't provide one. I can look into how make the documentation less confusing by adding a page exclusively dedicated to explain the problem it solves. Would that work for you? * 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.
The tutorials should be as small as possible to not scary the users. But I can put a comment "you should avoid using namespace". Does it help? * Your reference section is one very long thin list. You might want
to use more horizontal space.
I'd be happy already if I knew hot to remove the TOC from the reference page. It'd be much more clean already. * 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.
I assume I don't need to reply these questions during the review, given the reviewers may already be experienced on the answers. However, these are indeed good questions that I could add to the "design choices" page. Thank you. 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.
I don't quite understand this point. * 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.
These values will change when I replace the parser used now. Also, they can change based on the buffer sizes and parser options, which the user is/will be able to change. Nevertheless, I do want to do some serious benchmarking, specially because it'll give me a reference point when I write my own parser and want to compare performance speedup. I can do some poor benchmark this weekend and show the results. * 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.
That's a good idea, thank you. I won't replace the current tutorial, though, as it teaches the main players on HTTP. I can, however, add a second tutorial showing how to implement a **simple** file server. * 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 right about being paranoid here. I was too. But I've spent some time researching HTTP/2.0 and I got confident about the current design. But you could vote for conditional acceptance based on HTTP/2.0 and I'd think it is fair. * 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 already started the experiments with the sanitizers, but I got a problem with Boost Test and the experiment was put on hold. I'll come back to it later. I hope all the above isn't too disheartening Vinícius. Not at all. I think you're the second person to dedicate more time reviewing my work and this isn't disheartening at all. Just that your feedback is usually around tooling and not the design itself (everybody has its preferences). If you took
six weeks extra to finish the port of Http to ASIO proper,
I disagree about this point. Boost.Http is properly using Asio. Just because it won't support standalone Asio, it doesn't mean it's improperly using Asio. added a
Jamfile for the main library build,
Consider this done. I won't integrate into Boost without removing CMake first. This was already partially stated in the Bjorn's announcement (but somehow implicit, to be fair). -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On 8/7/15 3:28 PM, Vinícius dos Santos Oliveira wrote:
Delay acceptance of the library will only delay its usage.
I would disagree here. There is no reason people can't start using it now. In fact, it's much easier to get a library accepted if people have already started to depend on it.
2. Before presenting for review
normal library builds cannot use cmake OR should be header only like Hana.
I don't think that there is a requirement that boost build be supported in order to review a library.
CMake will be replaced by Boost.Build before any integration.
I don't think it's necessary to remove CMake support. No reason you can't leave it in and have both CMake and Boost Build support.
Until you address a minimum of items 1 and 4, I am very sorry but I
must vote for rejection.
Picking a nit here. The review manager is under no obligation to weigh "votes" equally. So I think it would be better if we used the word "recommendation" rather than vote.
* 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.
The items listed in https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook do not represent any official boost policy nor have they been discussed such that they represent any consensus. So though they may be relevant to Nail's recommendation, they may not be relevant to anyone else's. Robert Ramey
On 8/7/2015 9:24 PM, Robert Ramey wrote:
On 8/7/15 3:28 PM, Vinícius dos Santos Oliveira wrote:
* 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.
The items listed in https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook do not represent any official boost policy nor have they been discussed such that they represent any consensus. So though they may be relevant to Nail's recommendation, they may not be relevant to anyone else's.
+1 Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
Agustín K-ballo Bergé wrote:
Robert Ramey wrote:
Niall Douglas wrote:
all the stuff from https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook.
The items listed in https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook do not represent any official boost policy nor have they been discussed such that they represent any consensus. So though they may be relevant to Nail's recommendation, they may not be relevant to anyone else's.
+1
+1. I'm not really sure why a page called "BestPracticeHandbook" that lives on http://*.boost.org/* exists when it is not a "Best practice handbook" and not anything that the Boost community agrees upon or even acknowledges. Glen
On 7 Aug 2015 at 22:18, Glen Fernandes wrote:
all the stuff from https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook.
The items listed in https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook do not represent any official boost policy nor have they been discussed such that they represent any consensus. So though they may be relevant to Nail's recommendation, they may not be relevant to anyone else's.
+1
+1. I'm not really sure why a page called "BestPracticeHandbook" that lives on http://*.boost.org/* exists when it is not a "Best practice handbook" and not anything that the Boost community agrees upon or even acknowledges.
It's a wiki document. Anyone with a Trac login can edit it. If you feel anything in there is technically wrong, fix it. If you feel anything in there shouldn't be in there, raise the issue here and if there is consensus it shouldn't be in there, we'll delete it. I would also add that library authors will pick and choose from the handbook as they see fit. It's not a gospel, nor claims to be one. The key thing Boost needed was a library development resource guide as we had little which was up to date and especially relevant to C++ 11/14. I wrote my personal vision of that, as I was the one who invested the 120 hours or so of my family time to write it and I think that investment of my time gave me that right for the first edition. If you, or anyone else being critical of my substantial effort invested here, would like to write something better then go ahead and do it instead of +1 attacking it when you haven't done anything better yourself. But I'd prefer if it evolves into something more consensual, and as I mentioned everybody has editing rights. That indeed was the original intent - to get the ball rolling on some new shared documentation, as nobody was investing the effort to improve Boost documentation at that level in recent years. You may note each section in the wiki document has a comments section in which people can ask questions, or point out mistakes or otherwise give feedback per-section. That feedback system has already proven useful in substantially improving some of the sections in the guide where what I wrote was technically wrong, confusing or unclear, so I guess some people are finding the Handbook useful. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 8/8/2015 1:47 PM, Niall Douglas wrote:
On 7 Aug 2015 at 22:18, Glen Fernandes wrote:
all the stuff from https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook.
The items listed in https://svn.boost.org/trac/boost/wiki/BestPracticeHandbook do not represent any official boost policy nor have they been discussed such that they represent any consensus. So though they may be relevant to Nail's recommendation, they may not be relevant to anyone else's.
+1
+1. I'm not really sure why a page called "BestPracticeHandbook" that lives on http://*.boost.org/* exists when it is not a "Best practice handbook" and not anything that the Boost community agrees upon or even acknowledges.
The key thing Boost needed was a library development resource guide as we had little which was up to date and especially relevant to C++ 11/14. I wrote my personal vision of that, as I was the one who invested the 120 hours or so of my family time to write it and I think that investment of my time gave me that right for the first edition. If you, or anyone else being critical of my substantial effort invested here, would like to write something better then go ahead and do it instead of +1 attacking it when you haven't done anything better yourself.
Why don't you submit it for the traditional review instead? After all, having spent months (or years!) of your personal time on a library don't automatically entitle you to make it a Boost library.
But I'd prefer if it evolves into something more consensual, and as I mentioned everybody has editing rights. That indeed was the original intent - to get the ball rolling on some new shared documentation, as nobody was investing the effort to improve Boost documentation at that level in recent years. You may note each section in the wiki document has a comments section in which people can ask questions, or point out mistakes or otherwise give feedback per-section.
That feedback system has already proven useful in substantially improving some of the sections in the guide where what I wrote was technically wrong, confusing or unclear, so I guess some people are finding the Handbook useful.
I can't speak for others, but I don't think anyone claimed the Handbook isn't useful. For me personally, the implications that this is a Boost blessed document rubs me the wrong way, just as libraries that call themselves Boost.Whatever without ever having been reviewed do. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On 8/8/15 10:06 AM, Agustín K-ballo Bergé wrote:
I can't speak for others, but I don't think anyone claimed the Handbook isn't useful. For me personally, the implications that this is a Boost blessed document rubs me the wrong way, just as libraries that call themselves Boost.Whatever without ever having been reviewed do.
lol - you're speaking for me! I just wanted to point out the fact that the "best practices" doesn't speak for boost. It's not that it says it does - but the way it's presented might lead casual readers to assume that it does. I can't really object to Nial's making his personal recommendations available in this way. After all, I've done essentially the same thing in www.blincubator.com. I'm just concerned that that placement and tone suggest that it's the result of some consensus when it's not. Of course one could argue that no one has had sufficiently strong opinion to update/comment or whatever - but this is unconvincing to me. Most of don't have time to argue against everything we don't agree with. This seems like it's an exploit the boost "trademark" to enhance credibility and audience beyond what it would otherwise have. I don't want to dwell too much on the "best practices" wiki. I'm more interested in looking at the larger picture of "Boost Web Presence". There are a couple of things going on here. a) There are many more opportunities to promote the Boost Mission than there used to be. b) There exists the ability to better integrate and promote user driven content (such as Nail's). c) How do we better integrate issue tracking to make it easier to use and integrate with Modular Boost d) there is the isocpp website - how should Boost complement, collaborate, or perhaps counter this effort. e) Boost content has grown topsy turvy over the last 15 years. There's lot's of stuff that's outdated, lot's of stuff that's really useful but hard to find. How do we organize all this information in a more coherent way? And what technology can we leverage on to make it easier to keep upto date. f) Rene Rivera has been responsible for Boost infrastructure for many years and has been very, very reliable in this area. But the requirements for evolution of boost web presence have increased by a lot. I notice that Rene has been taking on more responsibilities on boost predef which is a not trivial exercise. Note all this is in addition to the being responsible for boost build/test infrastructure. Also I believe that he has a new job as well. I contacted Rene about offloading the responsibility for the Boost Web presence and he seemed receptive to the idea. g) We had a meeting about this at C++Now and a weak consensus was reached that we should be looking to evolve Boost Web Presence. Michael Caisse generously agreed to lend resources of his organization to help with this effort. It seems that his attempts to move in this direction have been stymied due to forces beyond his control. I would like to see progress on this front. h) I had all this in mind when I made the boost library incubator. This might be consider a place where ideas for Boost 2.0 are being prototyped. Some ideas have been a disappointment, others seem useful. I'm not sure of the future of the incubator - but for now it's not urgent to decide as, once up and running, it's been surprisingly easy to maintain. All in all, the boost web presence looks to me that it's stuck in 1999. Reading this, I'm not sure it should be posted here - but then I'm not sure where it should be posted. Robert Ramey
On 8 Aug 2015 at 10:48, Robert Ramey wrote:
g) We had a meeting about this at C++Now and a weak consensus was reached that we should be looking to evolve Boost Web Presence. Michael Caisse generously agreed to lend resources of his organization to help with this effort. It seems that his attempts to move in this direction have been stymied due to forces beyond his control. I would like to see progress on this front.
My memory was this was actually a strong unaminous consensus. The only benefits of the current setup of static HTML and Trac is it is virtually impossible to hack into, and therefore the maintenance cost is very low - so low we've lost root access because nobody needed root access for so long. I remember strongly advocating that any alternative to the existing setup needed support and maintenance costs factored into the decision matrix, and I had understood that was exactly what Michael was going to go investigate so he could give us some options. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 8 Aug 2015 at 14:06, Agustín K-ballo Bergé wrote:
The key thing Boost needed was a library development resource guide as we had little which was up to date and especially relevant to C++ 11/14. I wrote my personal vision of that, as I was the one who invested the 120 hours or so of my family time to write it and I think that investment of my time gave me that right for the first edition. If you, or anyone else being critical of my substantial effort invested here, would like to write something better then go ahead and do it instead of +1 attacking it when you haven't done anything better yourself.
Why don't you submit it for the traditional review instead? After all, having spent months (or years!) of your personal time on a library don't automatically entitle you to make it a Boost library.
A traditional review doesn't suit shared documentation: everybody will have their own opinion, nobody wants to do the work to merge opinions, it all degrades into a bitching about other people's opinions match which ultimately is resolved by a single person ignoring everybody else and implementing something which nobody much likes, but it is what it is: a piece of shared infrastructure. We've been though all this before. Dave went through this more than enough times. The people who maintain Boost.Build and Boost.Test and the web services see lots of vitriol spouted against their efforts and design choices here and elsewhere, and most of us here have felt frustration with the Boost infrastructure at one time or another. The tl;dr; of the whole thing is that Boost is not strong at shared infrastructure. We don't have the culture, processes nor resourcing to do shared infrastructure or shared decision making well. I have proposed many ways of fixing that over the past three years, but to put it charitably there is not community consensus on what to do. The Steering Committee can not therefore allocate funding. We continue therefore to do nothing, and therefore nothing changes.
I can't speak for others, but I don't think anyone claimed the Handbook isn't useful. For me personally, the implications that this is a Boost blessed document rubs me the wrong way, just as libraries that call themselves Boost.Whatever without ever having been reviewed do.
The very first sentence makes it very clear it is not a Boost blessed document. Moreover, the target audience for that document is the tiny number of people wanting to author a Boost library. They tend to be a highly sophisticated audience who are subscribed to this list. It's not like any Boost library author is going to get misled about recommendations in that document being requirements to enter Boost. And again, I repeat that that document is on the wiki, and is editable by anyone with a Trac login. That alone makes it not like a Boost library. It's the same as any other content on the Boost wiki - probably outdated but maybe useful information for Boost library authors and users and the Googlebot. For the record I personally would far prefer a situation where Boost employed a full time permanent infrastructure and maintenance engineer whose full time day job is to improve and keep up to date all the shared Boost infrastructure. Individual people like me would still lead out the substantial effort in writing the first draft of some piece of shared documentation like that Handbook, but the full time maintenance engineer would de-Niall the shared document into something more generic and less personally written. But without someone independent and acting not according to their own opinion and with enough free time to do that rewrite, we're stuck. I'm going to defend - hard - my opinions expressed in that document. You're going to attack the opinions you disagree with hard - and probably not budge me an inch. That's why you need an independent editor in between to do the rewrite according to their best guess as to consensus. And we struggle to find review managers for maybe two dozen hours of managing a review, let alone the 80+ hours it would require to rewrite that handbook after multiple rounds of review and feedback to create a consensus document. And then what about someone to maintain the thing and keep it up to date after so much shared effort has been invested? There is no point in proceeding with a shared document review until we solve the shared infrastructure maintenance problem first. It would be wasted effort for all. So we've got what we've got, and neither you nor I nor anybody is entirely happy with the outcome, same as for all shared Boost infrastructure. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 8/11/15 2:11 PM, Niall Douglas wrote:
On 8 Aug 2015 at 14:06, Agustín K-ballo Bergé wrote:
The very first sentence makes it very clear it is not a Boost blessed document. Moreover, the target audience for that document is the tiny number of people wanting to author a Boost library. They tend to be a highly sophisticated audience who are subscribed to this list. It's not like any Boost library author is going to get misled about recommendations in that document being requirements to enter Boost.
1. "The very first sentence" was added today. 2. google 'boost best practices' search brings up your link at the very top. 3. Sophisticated or not, the last thing Boost needs is people being turned off by all the gobbledygook on it.
On 8/11/2015 3:11 PM, Niall Douglas wrote:
On 8 Aug 2015 at 14:06, Agustín K-ballo Bergé wrote:
I can't speak for others, but I don't think anyone claimed the Handbook isn't useful. For me personally, the implications that this is a Boost blessed document rubs me the wrong way, just as libraries that call themselves Boost.Whatever without ever having been reviewed do.
The very first sentence makes it very clear it is not a Boost blessed document.
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it. I'd really appreciate if you'd do something of the sort yourself in your upcoming emails.
[snip]
I did not read the rest of the message, I'm sorry if you spent any of your family time on it. I'm sure you'll understand though, as I too have family. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
Oleg Grunin wrote:
1. "The very first sentence" was added today. 2. google 'boost best practices' search brings up your link at the very top. 3. Sophisticated or not, the last thing Boost needs is people being turned off by all the gobbledygook on it.
Agustín K-ballo Bergé wrote:
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it. I'd really appreciate if you'd do something of the sort yourself in your upcoming emails.
I added the note[1], after Niall's e-mail, encouraged by his words about editing the wiki. I believe Niall was referring to the "originally written by Niall Douglas" sentence. Niall: I hope the insertion of the note (and the removal of the word "originally") is acceptable. I'm not as passionate about sharing ideas about best practices in C++ as you are but it is important to me that people interested in contributing to Boost are not mislead and think that your document speaks for Boost, any Boost community member, or is mandate in any way. Glen [1] No "employed full time permanent infrastructure and maintenance engineer whose full time day job is to improve and keep up to date all the shared infrastructure" was harmed in the writing of this note. -- View this message in context: http://boost.2283326.n4.nabble.com/http-Formal-review-of-Boost-Http-tp467860... Sent from the Boost - Dev mailing list archive at Nabble.com.
On 12 Aug 2015 at 6:35, Glen Fernandes wrote:
Agustín K-ballo Bergé wrote:
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it. I'd really appreciate if you'd do something of the sort yourself in your upcoming emails.
I added the note[1], after Niall's e-mail, encouraged by his words about editing the wiki. I believe Niall was referring to the "originally written by Niall Douglas" sentence.
Thanks for the edit. Now you're an author of the handbook too.
Niall: I hope the insertion of the note (and the removal of the word "originally") is acceptable. I'm not as passionate about sharing ideas about best practices in C++ as you are but it is important to me that people interested in contributing to Boost are not mislead and think that your document speaks for Boost, any Boost community member, or is mandate in any way.
I felt your note inappropriately phrased, as not all the content is mine and certainly what it links to is definitely not by me. For example, the section on input fuzz testing came from Chandler, even if I elaborated up the words in that section (which is why it is so short - as a general rule, if it links to my code it's my opinion, if it doesn't link to my code it isn't). Another section not mine is the one on online compilers wandbox etc, that idea and much of the content came from Krzysztof and Louis. I replaced your edit with this instead: "The views and opinions expressed in this article are those of its authors and those who contributed feedback, code, links and ideas. This article does not reflect the official policy nor position of Boost, nor necessarily any one Boost library author, library maintainer, or community member." Is everyone happy with this phrasing? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
"The views and opinions expressed in this article are those of its authors and those who contributed feedback, code, links and ideas. This article does not reflect the official policy nor position of Boost, nor necessarily any one Boost library author, library maintainer, or community member."
It's getting better. I replaced it with: "The views and opinions expressed in this article are those of its authors. This article does not reflect the official policy nor position of Boost, nor necessarily any Boost library author, library maintainer, or community member." If you wish to share responsibility for any of the views and opinions that you've expressed in the article (with any of the people that contributed ideas or code to you) then maybe you should add a section to the article and credit those people by name. Otherwise, that wording does not belong in the disclaimer.
Thanks for the edit. Now you're an author of the handbook too.
You're welcome. I'm happy for the article to note that I contributed the disclaimer that tells people that you don't speak for Boost or its community. Best, Glen -- View this message in context: http://boost.2283326.n4.nabble.com/http-Formal-review-of-Boost-Http-tp467860... Sent from the Boost - Dev mailing list archive at Nabble.com.
Dear boosters, I really don't know how to say this, but... Could you please create another thread for your Wiki debate? I have an alert for this thread, since I'm really interested in Boost.HTTP review, but not on particular fights. I believe it's disrespectful for Vinicius, Bjorn and people who wish to read about the review. Sorry for having to write this, hope you can understand. Rodrigo Madera
On 12 Aug 2015 at 9:49, Agustín K-ballo Bergé wrote:
The very first sentence makes it very clear it is not a Boost blessed document.
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it. I'd really appreciate if you'd do something of the sort yourself in your upcoming emails.
Firstly I didn't write that note. Which proves it's a wiki document editable by anyone. Secondly I'm not going to write a disclaimer every single time I refer to that handbook as it's a shared document editable by anyone. It's there precisely to save wasted time writing the same email over and over again. It's there to help library authors get a head start on getting their library Boost-ready. Why would you disclaim with a warning every time you link to the Boost wiki?
[snip]
I did not read the rest of the message, I'm sorry if you spent any of your family time on it. I'm sure you'll understand though, as I too have family.
You might want to consider your tone. That was a real passive aggressive dick thing to say. I invest considerable unpaid effort into writing email, and it's a difficult choice when I would prefer to kick a ball with my child instead. You don't have to read what I write, but trivialising the fact I wasted my family time to reply to your mean spirited whining isn't on. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 8/12/2015 12:27 PM, Niall Douglas wrote:
On 12 Aug 2015 at 9:49, Agustín K-ballo Bergé wrote:
The very first sentence makes it very clear it is not a Boost blessed document.
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it. I'd really appreciate if you'd do something of the sort yourself in your upcoming emails.
Secondly I'm not going to write a disclaimer every single time I refer to that handbook as it's a shared document editable by anyone. It's there precisely to save wasted time writing the same email over and over again. It's there to help library authors get a head start on getting their library Boost-ready. Why would you disclaim with a warning every time you link to the Boost wiki?
The potential for perceiving the Best Practice Handbook as a Boost document is what lead others to feel the need to clarify the situation. Since I don't think the intention is to mislead people into taking that document as Boost's Best Practice Handbook just for being in Boost's wiki, I'd appreciate any and all efforts to mitigate that. Though as I said, I am happy with (just) the note.
[snip]
I did not read the rest of the message, I'm sorry if you spent any of your family time on it. I'm sure you'll understand though, as I too have family.
You might want to consider your tone. That was a real passive aggressive dick thing to say. I invest considerable unpaid effort into writing email, and it's a difficult choice when I would prefer to kick a ball with my child instead. You don't have to read what I write, but trivialising the fact I wasted my family time to reply to your mean spirited whining isn't on.
I did not mean to trivialize your time nor how you use it, my apologies if it seems that way. You say it best, I do not have to read what you write, but I have asked you in the past to consider the time it takes others to read your messages as important as the time it takes you to write them, so for this unadorned effective reply I thank you. Regards, -- Agustín K-ballo Bergé.- http://talesofcpp.fusionfenix.com
On August 12, 2015 11:27:09 AM EDT, Niall Douglas
On 12 Aug 2015 at 9:49, Agustín K-ballo Bergé wrote:
The very first sentence makes it very clear it is not a Boost blessed document.
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it.
I think everyone is reasonably happy on this point. Let's call a moratorium on it. ___ Rob (Sent from my portable computation engine)
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Rob Stewart Sent: 13 August 2015 00:09 To: boost@lists.boost.org Subject: Re: [boost] [http] Formal review of Boost.Http
On August 12, 2015 11:27:09 AM EDT, Niall Douglas
wrote: On 12 Aug 2015 at 9:49, Agustín K-ballo Bergé wrote:
The very first sentence makes it very clear it is not a Boost blessed document.
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it.
I think everyone is reasonably happy on this point. Let's call a moratorium on it.
+1 And even more constructively, let's have more contributions (possibly with differing views) to this and other documents on Trac (for want of somewhere better to put them). Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 (0) 1539 561830
On 13-Aug-15 2:09 AM, Rob Stewart wrote:
On August 12, 2015 11:27:09 AM EDT, Niall Douglas
wrote: On 12 Aug 2015 at 9:49, Agustín K-ballo Bergé wrote:
The very first sentence makes it very clear it is not a Boost blessed document.
I'm happy with the newly added note claiming that those are your opinions and yours only. That way others won't feel the need to clarify that's not a Boost thing every single time you mention it.
I think everyone is reasonably happy on this point. Let's call a moratorium on it.
Can we first drop the link to that document from http://www.boost.org/development/requirements.html#Guidelines though, or at least move it below other pre-existing recommendations, including such important ones as naming? - Volodya
On 12 Aug 2015 at 16:27, Niall Douglas wrote:
I did not read the rest of the message, I'm sorry if you spent any of your family time on it. I'm sure you'll understand though, as I too have family.
You might want to consider your tone.
I apologise to the list for losing my temper in this earlier message. I appreciate it's the third time this year, and I am very sorry about that. All I can say is I shall try to do better. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
If you feel anything in there shouldn't be in there, raise the issue here and if there is consensus it shouldn't be in there, we'll delete it.
Why look for consensus to modify it now? Shouldn't that have happened before the document was even published on the Boost Trac?
I wrote my personal vision of that, as I was the one who invested the 120 hours or so of my family time to write it and I think that investment of my time gave me that right for the first edition. If you, or anyone else being critical of my substantial effort invested here, would like to write something better then go ahead and do it instead of +1 attacking it when you haven't done anything better yourself.
What nonsense. What would you think would happen if had admin access to Boost, just committed AFIO without a formal review, and then suggested that changes happen to it only after consensus? Is this how you're going to respond during the formal review of AFIO? By crying "Don't attack AFIO when you haven't written your own asynchronous file I/O library"?
But I'd prefer if it evolves into something more consensual, and as I mentioned everybody has editing rights.
How about we rename the page from "Best Practice Handbook" to "Niall Douglas' Best Practice Handbook" (or"Niall Douglas' Links To Examples Of Best Practice For C++ 11/14 Libraries" so that page name matches the title)? Just so that nobody is in danger of considering it Boost blessed just because it lives on the Boost Trac. Maybe later when sufficient Boost community involvement shapes it into a document worthy of that former title, we can rename it back?
You may note each section in the wiki document has a comments section in which people can ask questions, or point out mistakes or otherwise give feedback per-section. That feedback system has already proven useful in substantially improving some of the sections in the guide where what I wrote was technically wrong, confusing or unclear, so I guess some people are finding the Handbook useful.
The only piece of feedback on it that I see is one comment and that questions whether this should be on your blog instead. [1] [1] https://muut.com/cppbestpractice/#!/strongly-consider-using-git Best, Glen
Vinícius dos Santos Oliveira wrote:
I'd be happy already if I knew hot to remove the TOC from the reference page. It'd be much more clean already.
I just chose to avoid nesting too many [section] blocks in favor of [heading] at that level: - https://github.com/boostorg/align/blob/master/doc/align.qbk#L528 - http://boostorg.github.io/align/align/reference.html Best, Glen
Hello community, I'm not done playing with Boost.HTTP yet to give deeper usage insight, but I want to throw in some things I have already noticed that really concern me. I do agree that, from a logical point of view, Vinicius is right in saying that "It's not against the rules" to do something with a rather lower bar than the same thing done right, I still believe it's against Boost's stablished quality. Allow me to explain: - The library should be header only While (strictly speaking) it's not a Boost requirement, there is no strong reason here to require a compiled library. Since Boost.ASIO is header only, and since Boost.HTTP is technically an extension of Boost.ASIO, it should model it and be completely header only. According to the documentation page [1], the library isn't header-only because it uses a borrowed C parser, because it creates it's own error codes, and pretty much just those two. The first one could be solved using Boost.Spirit (which is on the Roadmap). The error categories, I would just ask this: How did ASIO solve it? I'm not against Boost libraries depending on one another if there is a clear advantage over it, so Boost.Spirit would be a great option for parsing. I never did more than the basic HTTP header parsing, so simple regex works great for my current use cases. I don't think parsing HTTP would be that much of a trouble using Spirit. - The library doesn't really need C++11 Boost.HTTP only uses C++11 for the convenience of enum classes and lambdas [2]. This by no means is justified requirement of C++11 functionality in a Boost Library, except for Proof of Concept work and prototyping. Once the library is sound, it should go to C++03 or even C++98 if at all possible. Now let me be clear: I know some will just jump on the "Boost should now push the compiler boundaries" bandwagon, but that's not how I remember Boost to be. I understand the recently added Boost.Hana is groundbreaking and all, but IMHO these kind of experimental libraries shouldn't be the norm. I was tempted to vote No to Hana approval (despite it's greatness) just because of that. To me, Boost is a complementary set of C++ libraries that went a far way into giving full compiler support for even the most stubborn (and archaic) of compilers. Even if it meant implementing several workarounds. I can relate to the usefulness of such support, having worked at a Fortune 500 with an ancient environment that supported (and I believe still does) millions of customers. Boost supported it thanks to countless hours of devotion from library developers to make it work with a great set of compilers. Now I'm not saying that we should continue to support C++08 for every new library. Of course not. But we should make it a requirement if (1) abstractions are readily available without significant costs and (2) they don't offer significant advantage over older c++ versions. Both (1) and (2) are available on the abstraction layer offerred by ASIO. And since Boost.HTTP *is* a clear extension of ASIO, I expect it to use it. It's what the users should expect just by looking at it. And best of all? This is exactly Vinicius thoughts according to the Roadmap. So this should be done now. - Maybe it should be (Boost.)Asio.Extensions? Boost.HTTP is an extension to Boost.ASIO (and hopefully for non-Boost ASIO as well in the near future). Why not create a Boost.AsioProtocols (or Boost. AsioExtensions, whatever) and go around creating several libraries for it? In the future we might have other protocols on top of ASIO. Boost.FTP, Boost.STMP, Boost.Torrent, etc. They could all go into a protocol extension library, since it's basically message parsing on top of Boost.ASIO. What do you guys think about this? - Ignoring ASIO Abstraction Again, and already pointed out by Niall, Boost.HTTP should use ASIO's abstraction. A lot of work and thought has been put into this mechanism. Ignoring it in a library that extends it is not an option from my point of view. Boost.HTTP must support and use consistently all Boost.ASIO's mechanisms for abstraction. If I wish to use non-Boost ASIO, technically, Boost.HTTP should work. What could be a reason not to support this is ASIO has the abstraction ability? - High-level Usage I was surprised to see the low level of Boost.HTTP's API. At first I disliked it, but then it seemed natural for the Boost.ASIO mindset. This makes me believe even more firmly in a possible Boost.Asio extension model instead of standalone library. However, for me (and I bet several users) reading the library name "Boost.HTTP" would seem to be a higher level API. Instead, this looks as Boost.ASIO.HTTP. Which brings me back to preffer Boost.AsioExtensions. Maybe a higher level abstraction for simple use cases? Something like resource routing and interception capabilities, maybe. I'm not yet sure I wish to write HTTP code this low level. - Benchmarks How does Boost.HTTP perform against, lets say, Boost.ASIO's several HTTP server examples? Should I really use Boost.HTTP instead of some simple as possible layers typical of embedded servers? What's the cost? Graphs would be most welcome. Users want to be convinced that they will get an edge by choosing Boost.HTTP. - Examples At the moment, there is only one example [3]. Examples are the very first thing I look at for Boost libraries. We need more. I talked to Vinicius and he pointed me to deleted examples that might work as a base [4]. Examples for handler based communications, coroutines, continuation, chunked, file server (included in the library) should be available again. Boost.ASIO's documentation is also great, and could be an inspiration. Same goes for Tutorial. - HTTP 2.0 Will it be added in Boost.HTTP? Will it be a separate library (Boost.HTTP2)? - Haste to Review As it is right now I see that http is urging haste to be boost under the argument that "other things are not requirements". I would suggest seeing this rather as a craftsman art job rather than a legally conforming implementation on what is expected and "required" for boostification. Boost was never about requirements. It's about C++ cleverness. The good kind. It's about making good, outstanding libraries with the maximum quality possible. Not about meeting minimums. I'd choose any Boost library over any other in the market 9 out of 10 times. The library is very near completion. It provides a layer that most applications will use to communicate. I myself am needing it right now to substitute my hand made provisions. I really want Boost.HTTP, but it needs more work. I'm pretty sure Vinicius will deal with these little items in no time. I know him from our online Brazilian C++ users group, and I know it won't be much of a challenge for him. He's really good and it's not his first HTTP library. Win/win situtation. Best regards, Rodrigo Madera [1] http://boostgsoc14.github.io/boost.http/design_choices.html [2] http://boostgsoc14.github.io/boost.http/design_choices.html [3] https://github.com/BoostGSoC14/boost.http/tree/master/example [4] https://github .com/BoostGSoC14/boost.http/commit/21745f8dc81206f22445e43f9ab71a632c5a767f#diff- e3f28b5e7e2044a2e6332a5e997eabbaL86
2015-08-11 17:26 GMT-03:00 Rodrigo Madera
[...]
Many of the Rodrigo Madera's concerns were addressed by the design choices chapter. I'm glad this chapter proved to be useful, so I don't need to repeat myself and users can lose their doubts without even leaving the Boost.Http documentation. - Maybe it should be (Boost.)Asio.Extensions?
Boost.HTTP is an extension to Boost.ASIO (and hopefully for non-Boost ASIO as well in the near future). Why not create a Boost.AsioProtocols (or Boost. AsioExtensions, whatever) and go around creating several libraries for it?
In the future we might have other protocols on top of ASIO. Boost.FTP, Boost.STMP, Boost.Torrent, etc. They could all go into a protocol extension library, since it's basically message parsing on top of Boost.ASIO.
What do you guys think about this?
Boost.Http is done from the very beginning to allow alternative backends (like I stated on my reply to Niall just a few minutes ago, with much more points tackled). Some backends might not even do network or need Asio (shared memory among process or other means). This feature is important for HTTP applications. OTOH, Boost.Http is fully async and uses Asio abstraction to do asynchronous code. I've already played with several asynchronous frameworks that use different approaches (Qt, Node.js, futures...) and I'm very impressed by Asio proposal. Boost.Http will always depend on Boost.Asio to abstract its asynchronous nature. Anyway, to not delay my opinion further. It's not that I'm very against putting Boost.Http into Asio.Extensions or alike. I'm against putting Boost.Http into a mix of network protocol libraries like FTP, SMTP and others. The reason I'm against is because it may trick the user to think that this is (1) merely a protocol implementation and (2) there will be network activity when Boost.Http doesn't even need network activity (e.g. shared memory based backends). - High-level Usage
I was surprised to see the low level of Boost.HTTP's API. At first I disliked it, but then it seemed natural for the Boost.ASIO mindset. This makes me believe even more firmly in a possible Boost.Asio extension model instead of standalone library.
However, for me (and I bet several users) reading the library name "Boost.HTTP" would seem to be a higher level API. Instead, this looks as Boost.ASIO.HTTP. Which brings me back to preffer Boost.AsioExtensions.
Maybe a higher level abstraction for simple use cases? Something like resource routing and interception capabilities, maybe.
I'm not yet sure I wish to write HTTP code this low level.
cpp-netlib, pion and many other provide request routers and server abstractions. They usually couple all abstractions together and use a design like: - There is the server+request-router+handler+thread-pool+... - Request factory, which the user will extend and pass to the above inflated abstraction. Even if the request object is decoupled from the server object, allowing you to implement your own server with your own needs, support for alternative HTTP backends would be left out, which is wrong. The message-oriented abstraction in Boost.Http solves this problem. Node.js takes a different approach on the request router matter by providing none. This is awesome because then you can choose whatever style you prefer. You can register a tree of handlers to implement a ReST service. But then, maybe a tree-based request router is not appropriate to your application and you want a middleware-like request router. And then there are the rules the request router is going to use (resource/path/uri requested, authentication headers, arbitrary user-provided predicate). The request router itself might be seen as a handler and you might want to nest request routers. The thing here is that I'm very ambitious about this project and I estimate I'd take as long as the already spent time just to deliver the request router. I want to be sure that I'm providing the best request router possible to write. Something that could go into the C++ standard and nobody would fear to be deprecated in the future. A big challenge to the request router is that it can become rather inefficient if I put customization points everywhere. And if I think about performance, I remember that most of the request routers I saw allow you to change the rules at runtime when this feature is rarely needed. Maybe some TMP magic can help here. - Benchmarks
How does Boost.HTTP perform against, lets say, Boost.ASIO's several HTTP server examples?
Should I really use Boost.HTTP instead of some simple as possible layers typical of embedded servers? What's the cost?
Graphs would be most welcome. Users want to be convinced that they will get an edge by choosing Boost.HTTP.
I'd suggest to focus on correctness. It's very tricky to get a correct implementation that will portably work among several little know implementations. You just should not use an unmaintained copy & paste solution. - HTTP 2.0
Will it be added in Boost.HTTP?
Will it be a separate library (Boost.HTTP2)?
I'll try to provide a proof-of-concept tonight. It won't be Boost quality and I think I'll use nghttp2. But should be useful to help the job of the reviewers. - Haste to Review
As it is right now I see that http is urging haste to be boost under the argument that "other things are not requirements". I would suggest seeing this rather as a craftsman art job rather than a legally conforming implementation on what is expected and "required" for boostification.
Boost was never about requirements. It's about C++ cleverness. The good kind. It's about making good, outstanding libraries with the maximum quality possible. Not about meeting minimums. I'd choose any Boost library over any other in the market 9 out of 10 times.
The library is very near completion. It provides a layer that most applications will use to communicate. I myself am needing it right now to substitute my hand made provisions. I really want Boost.HTTP, but it needs more work.
I'm pretty sure Vinicius will deal with these little items in no time. I know him from our online Brazilian C++ users group, and I know it won't be much of a challenge for him. He's really good and it's not his first HTTP library. Win/win situtation.
I'm sorry if I left you guys under this impression. The first answer we got from the review was Niall's reply, focusing on features that would not even go on the Boost code or change the API design. This fact might have contributed to this view. These features aren't required for Boost and it'd be very weird to have a library rejected because these requirements. Also, there are features not implemented now that deserve further explanation to remove the impression that you guys got. This project can accumulate a lot of polemic decisions easily. I worked for quite awhile already and managed to finish the core of the library (and some extra features already like the possibly most amazing file server API that nobody here acknowledged/noticed ;-) ). I want this core into Boost as a confirmation that the design is right so far. And here I'm defending my design. And maybe the design is right as most (if not all) complaints are unrelated to the already proposed design. A high-level API will just manage the main players already exposed to remove the need for such amount of boilerplate code. However, I should state that I'm grateful for all the feedback this review is generating. Lots of points/hints on improvements to the library. And aren't obvious points, but things that make me think that a great deal of time was spent on the review. [1] http://nghttp2.org/ -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On Thu, Aug 13, 2015 at 9:53 AM, Vinícius dos Santos Oliveira < vini.ipsmaker@gmail.com> wrote:
2015-08-11 17:26 GMT-03:00 Rodrigo Madera
: [...]
Many of the Rodrigo Madera's concerns were addressed by the design choices chapter. I'm glad this chapter proved to be useful, so I don't need to repeat myself and users can lose their doubts without even leaving the Boost.Http documentation.
I'm actually challenging some of those answers. Please read again. The library, if accepted as is, doesn't have a guarantee that you will change it back to C++98/03. Same goes to a header-only design. But you're right. Let's focus on the design and implementation details for now. Boost.Http is done from the very beginning to allow alternative backends
(like I stated on my reply to Niall just a few minutes ago, with much more points tackled). Some backends might not even do network or need Asio (shared memory among process or other means). This feature is important for HTTP applications.
OTOH, Boost.Http is fully async and uses Asio abstraction to do
asynchronous code. I've already played with several asynchronous frameworks that use different approaches (Qt, Node.js, futures...) and I'm very impressed by Asio proposal. Boost.Http will always depend on Boost.Asio to abstract its asynchronous nature.
Boost.Http will always depend on Asio, then. So I still see the need to use full Asio abstractions and support for non-Boost Asio. Anyway, to not delay my opinion further. It's not that I'm very against
putting Boost.Http into Asio.Extensions or alike. I'm against putting Boost.Http into a mix of network protocol libraries like FTP, SMTP and others. The reason I'm against is because it may trick the user to think that this is (1) merely a protocol implementation and (2) there will be network activity when Boost.Http doesn't even need network activity (e.g. shared memory based backends).
If you can abstract it for HTTP, I can assume it can be done for others as well. Just take your abstractions and implement them for other protocols. That would still make Boost.Asio.Extensions interesting. The name Boost.Http by itself already proposes the idea of network traffic. Message abstraction with various representations (binary, textual) is a common pattern in networking code. I would expect any other protocol implementations to use it, and if possible, use different backends. cpp-netlib, pion and many other provide request routers and server
abstractions. They usually couple all abstractions together and use a design like:
- There is the server+request-router+handler+thread-pool+... - Request factory, which the user will extend and pass to the above inflated abstraction.
But this is good for a high-level API. As I stated, it would be nice to have both APIs. Have a lower level API and then an abstraction. Since you use Asio, you won't need all those details of cpp-netlib, but the rest could be compile time policies into a higher API.
The thing here is that I'm very ambitious about this project and I estimate I'd take as long as the already spent time just to deliver the request router. I want to be sure that I'm providing the best request router possible to write. Something that could go into the C++ standard and nobody would fear to be deprecated in the future.
Good! You should be. But we need it now to use Http in a real world application, or you will be leaving to users the responsibility of implementing Http code that should be inside the library already. Even if not the perfect solution at first.
A big challenge to the request router is that it can become rather inefficient if I put customization points everywhere. And if I think about performance, I remember that most of the request routers I saw allow you to change the rules at runtime when this feature is rarely needed. Maybe some TMP magic can help here.
Could you please elaborate why? As I see it, if you put the customized interceptors in place, only (and only) that request/response would "suffer". I agree that runtime rules are seldomly needed. In my experience, at least. - Benchmarks
I'd suggest to focus on correctness. It's very tricky to get a correct implementation that will portably work among several little know implementations.
What if the design is very far from others at the time? What if the basic Asio examples (with some processing that actually suffices some use cases) outperform severly? Since you have several competitors, benchmarks, to me, are important. Even if you design a perfect API with every abstraction possible, I wouldn't use it if it cut my TV streaming server performance even by a small percentage. It wouldn't be an advantage. I want to be sure that the abstraction won't cost me more than I'm willing to pay. And theoretically, there is no reason why Boost.Http it should underperform if enough static customization is allowed. You just should not use an unmaintained copy & paste
solution.
Sorry, I didn't understand. Do you mean to copy and paste benchmark code? - HTTP 2.0
I'll try to provide a proof-of-concept tonight. It won't be Boost quality and I think I'll use nghttp2. But should be useful to help the job of the reviewers.
Ok, so Boost.Http will have HTTP 2 support itself. Ok. I would love it if the syntax would remain (almost?) identical. - Haste to Review
I'm sorry if I left you guys under this impression. The first answer we got from the review was Niall's reply, focusing on features that would not even go on the Boost code or change the API design. This fact might have contributed to this view. These features aren't required for Boost and it'd be very weird to have a library rejected because these requirements.
We have been talking sporadically about your project since it was approved in GSoC, remember? I raised you my concerns on private messaging as well, way before the review started. While I agree with some points of Niall, I base my opinions on my own use cases. I download the tree, I compile it (when needed), read the examples, do the tests, and then raise my view. I'm not even done playing with Boost.Http. I see if I would have done it better. I see if it would make my work simpler. I see the reasoning. Please don't use this "Niall started it" as a shield to unacknowledge the problems pointed out. You didn't even respond to my initial points, disregarding them as someone else's. But if you read carefully, you will see that those are basic and important issues that affect the Boost quality I'm used to.
Also, there are features not implemented now that deserve further explanation to remove the impression that you guys got. This project can accumulate a lot of polemic decisions easily. I worked for quite awhile already and managed to finish the core of the library (and some extra features already like the possibly most amazing file server API that nobody here acknowledged/noticed ;-) ).
I found the documentation not enough for my testing of file_server. I had to PM you to get some example code on its use. If you would first have examples (as many as possible) then it would be better. I want this core into Boost as a confirmation that the design is right so
far. And here I'm defending my design. And maybe the design is right as most (if not all) complaints are unrelated to the already proposed design.
Don't discard points. Tackle them. I'll speed up my POC to make the deadline on my opinions of the library. But please respond to the points I raise. - Conclusion If you only wanted to review design, then a full library inclusion review wasn't needed. I just don't see it done *yet*. Regards, Rodrigo Madera
2015-08-13 19:17 GMT-03:00 Rodrigo Madera
If you can abstract it for HTTP, I can assume it can be done for others as well. Just take your abstractions and implement them for other protocols. That would still make Boost.Asio.Extensions interesting.
[...]
Message abstraction with various representations (binary, textual) is a common pattern in networking code. I would expect any other protocol implementations to use it, and if possible, use different backends.
Fair enough. Then, considering we're heading this path, what are the other implications? How many maintainers can we have per Boost library? It seems a hard job to allow only one maintainer to maintain the implementation of all protocols. What if ftp also have a socket? Will we type boost::asio::ftp::socket? Why not just boost::ftp::socket? Other points that are I'm missing? cpp-netlib, pion and many other provide request routers and server
abstractions. They usually couple all abstractions together and use a design like:
- There is the server+request-router+handler+thread-pool+... - Request factory, which the user will extend and pass to the above inflated abstraction.
But this is good for a high-level API.
Until the flaws start to show. We still see hate against iostream idea of mixing input/output and string format to this date. About the fine high-level abstraction: Can I use it in multi-threaded design? How are the threads going to be used? Will connections be shared among them or will be fine-grained requests shared? Which scheduling algorithm will be used? Can I provide my own thread-pool? What about other kinds of object pools (e.g. message pools)? Can I provide a custom allocator? Can I stop this server at anytime? What are the guarantees? These are just some initial questions. But the most problematic flaw in the design is the lack of consideration to alternative HTTP backends. If the library is accepted, I'm thinking to deliver features soon into a experimental namespace for which I give no guarantees related to API stability and slowly move them to stable API as I gather feedback and do more research. As I stated, it would be nice to have both APIs. Have a lower level API and
then an abstraction.
Yes, but the main players will be the same you already see here. You'd use a message object and a communication channel to respond to requests. Since you use Asio, you won't need all those details of cpp-netlib, but the
rest could be compile time policies into a higher API.
That looks like a good start.
The thing here is that I'm very ambitious about this project and I estimate
I'd take as long as the already spent time just to deliver the request router. I want to be sure that I'm providing the best request router possible to write. Something that could go into the C++ standard and nobody would fear to be deprecated in the future.
Good! You should be. But we need it now to use Http in a real world application, or you will be leaving to users the responsibility of implementing Http code that should be inside the library already.
Even if not the perfect solution at first.
Actually, if it's not a perfect solution at first, I'll end up either deprecating API or breaking API. I don't like either approach, so the remaining option is to deliver incrementally (not complete at first).
A big challenge to the request router is that it can become rather
inefficient if I put customization points everywhere. And if I think about performance, I remember that most of the request routers I saw allow you to change the rules at runtime when this feature is rarely needed. Maybe some TMP magic can help here.
Could you please elaborate why?
As I see it, if you put the customized interceptors in place, only (and only) that request/response would "suffer".
I agree that runtime rules are seldomly needed. In my experience, at least.
Well, your application routing rules might not need to touch the HTTP headers at all, just the request path/resource/uri. Then the code to match based on headers (this include authentication, session...) is just eating memory and CPU. This problem can be solved with TMP if the rules are static. As an hypothetical example, you might consider a rule like check_header("authorization" "Digest username=\"Mufasa\"..."). You have the request router object: router r; and you have routing rules: rule_t r1(/* ... */); handler h1(/* .... */); r.add(r1, h1); If we were to allow check_header rule, rule_t would need to store it somehow. But if you don't use this rule, memory and CPU is just being wasted. - Benchmarks
I'd suggest to focus on correctness. It's very tricky to get a correct implementation that will portably work among several little know implementations.
What if the design is very far from others at the time?
What if the basic Asio examples (with some processing that actually suffices some use cases) outperform severly?
Since you have several competitors, benchmarks, to me, are important.
Even if you design a perfect API with every abstraction possible, I wouldn't use it if it cut my TV streaming server performance even by a small percentage. It wouldn't be an advantage.
I want to be sure that the abstraction won't cost me more than I'm willing to pay.
And theoretically, there is no reason why Boost.Http it should underperform if enough static customization is allowed.
Fair enough. Now I'm convinced to compare against Asio HTTP examples, even if these two solutions aren't feature-comparable. You just should not use an unmaintained copy & paste
solution.
Sorry, I didn't understand. Do you mean to copy and paste benchmark code?
No, I meant the Asio HTTP examples adopted as a solution. Sorry for not being clearer in the earlier email. We have been talking sporadically about your project since it was approved
in GSoC, remember?
I raised you my concerns on private messaging as well, way before the review started.
Yes, it took a long time to polish the core. When you aim to allow alternative backends, you end up reading/researching a lot of specifications to make sure every use case will be possible. And this translate into small details on the specification (the concepts part of the documentation) that not many people would imagine it'd take a long time to get it done. While I agree with some points of Niall, I base my opinions on my own use
cases. I download the tree, I compile it (when needed), read the examples, do the tests, and then raise my view. I'm not even done playing with Boost.Http.
I see if I would have done it better. I see if it would make my work simpler. I see the reasoning.
And I'm very thankful about people like you that are dedicating a lot of time to give such meaningful feedback. I hope to deliver an awesome library to pay you guys back. Please don't use this "Niall started it" as a shield to unacknowledge the
problems pointed out. You didn't even respond to my initial points, disregarding them as someone else's. But if you read carefully, you will see that those are basic and important issues that affect the Boost quality I'm used to.
The points that I didn't commented were points that I didn't disagree. You were simple right in my view, so there is no need to answer anything. I'll re-read the whole thread after the review to gather points where the library can be improved and implement them. The only question I didn't answered yet was Darren Cook's about HTTP 2, for which I'm writing a prototype. - Conclusion
If you only wanted to review design, then a full library inclusion review wasn't needed. I just don't see it done *yet*.
Actually, I complain a lot about the design because that's where the breaking changes are, but I was obsessive with the implementation too. It's fair to state that I wasn't so obsessive with tooling, tough, as Niall noticed. However, tooling is improving (CI on several systems with some sanitizers are already being used). Even a conditional acceptance would be nice because then I'd have proved the proposed API is right and then I'd just need to improve completeness or implementation or any other point raised. And the small core proposed is intentional. You guys are already complained about little time to review such complex project. And I'm having a hard time defending this small core already. Imagine if I had proposed the polemic parts that I'm planning to implement. -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On Fri, Aug 7, 2015 at 2:08 AM, Bjorn Reese
Dear Boost and ASIO communities.
The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th.
Please answer the following questions:
1. Should Boost.Http be accepted into Boost? Please state all conditions for acceptance explicity.
My recommendation is no. It does not seem like it is in a 'finished' enough state to be accepted.
2. What is your evaluation of the design?
3. What is your evaluation of the implementation?
I don't feel qualified to comment on most of it. However, I think one seriously lacking feature is a set of convenience functions that could replace most of the boilerplate that is in the tutorial.
I like the travis & appveyor badges on the github landing page. I think that at a minimum some of the 'roadmap' items need to be pulled in before approval. Specifically: * Bjam (keep CMake for those who want it) * C++03/98 * Header only and compiled library * HTTP2 Some more that seem important, but I'm not sure: * Cookies - this seems like a pretty basic server functionality. Is this a hard thing to add? * Forms/File Uploads - Does this imply that it doesn't support POST? I feel like the initial implementation should at least support GET & POST.
4. What is your evaluation of the documentation?
I think the documentation needs serious work. The only example is very long and drawn out. The first example should be something a user can get running in a few lines of code, not a full page. There should also be more examples of how to integrate the http service with getting files from the file system, generating responses CGI-style, supporting TLS (is this a feature? If not this would be a show-stopper), etc. The documentation seems like it is missing a chunk between the tutorials/examples and the reference section where it enumerates the various options and settings that can be used to configure the library.
5. What is your evaluation of the potential usefulness of the library?
I think this is one of the *most* potentially useful libraries boost has reviewed in recent memory. That is why it is much more important that it be in a solid state right off the bat. I think that if this is released, we will immediately get a good amount of user interest. If they were to try using it like this, I expect they would be disappointed and possibly not try again in a few versions when it was fixed up.
6. Did you try to use the library? With what compiler? Did you have any problems?
No.
7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I spent a couple hours reading the documentation and browsing the source code.
8. Are you knowledgeable about the problem domain?
Not particularly. Tom Kent
2015-08-14 19:42 GMT-03:00 Tom Kent
Some more that seem important, but I'm not sure: * Cookies - this seems like a pretty basic server functionality. Is this a hard thing to add?
Cookies by themself are not so useful. You usually will want a session abstraction. A good session abstraction is tricky to get. Dynamic languages have the freedom to replace/inject anything they want, so they almost always get the features. If you aren't careful, you'll end up with two APIs, one for server-side data sessions and client-side data sessions. And there other points to study (different session backends like DB-baked, memory-baked...). * Forms/File Uploads - Does this imply that it doesn't support POST? I feel
like the initial implementation should at least support GET & POST.
POST is supported, but if you handle the message format used by web browsers, you'll have to parse the body yourself. A parser for forms and file uploads should be added to ease our lives. [...] supporting TLS (is this a feature?
If not this would be a show-stopper) [...]
Yes, TLS is supported. I even tried to advertise on the front page: https://boostgsoc14.github.io/boost.http/ -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On Sat, Aug 15, 2015 at 5:16 AM, Vinícius dos Santos Oliveira < vini.ipsmaker@gmail.com> wrote:
2015-08-14 19:42 GMT-03:00 Tom Kent
: Some more that seem important, but I'm not sure: * Cookies - this seems like a pretty basic server functionality. Is this a hard thing to add?
Cookies by themself are not so useful. You usually will want a session abstraction. A good session abstraction is tricky to get. Dynamic languages have the freedom to replace/inject anything they want, so they almost always get the features. If you aren't careful, you'll end up with two APIs, one for server-side data sessions and client-side data sessions. And there other points to study (different session backends like DB-baked, memory-baked...).
I guess that's where my lack of understanding of the subject matter comes in. I just assumed that the http layer would simply provide the cookies for a request, then the application's layer would handle making sessions.
* Forms/File Uploads - Does this imply that it doesn't support POST? I feel
like the initial implementation should at least support GET & POST.
POST is supported, but if you handle the message format used by web browsers, you'll have to parse the body yourself. A parser for forms and file uploads should be added to ease our lives.
This *must* be explained in the documentation!
[...] supporting TLS (is this a feature?
If not this would be a show-stopper) [...]
Yes, TLS is supported.
I even tried to advertise on the front page: https://boostgsoc14.github.io/boost.http/
I now see the introduction that says something like 'SSL is supported'. This needs to be seriously expanded in the documentation. At the very minimum there should be an example showing how to setup a TLS connection. Tom
On 15-Aug-15 3:19 PM, Tom Kent wrote:
I even tried to advertise on the front page: https://boostgsoc14.github.io/boost.http/
I now see the introduction that says something like 'SSL is supported'. This needs to be seriously expanded in the documentation. At the very minimum there should be an example showing how to setup a TLS connection.
+1. I think that saying "SSL" might be confusing these days, since even the last version called SSLv3 now is widely rejected by webservers. Saying TLS might be clearer. And indeed, implementation example seems like a must to me. While ASIO documentation include an example of SSL server, it's 2 pages of boilerplate, and hopefully it's not necessary to repeat all of that if I use a library specialized for HTTP servers. Thanks, Volodya
A quick reminder that the formal review ends on Sunday (August 16th.) Please consider submitting a review where the following questions are answered: 1. Should Boost.Http be accepted into Boost? Please state all conditions for acceptance explicity. 2. What is your evaluation of the design? 3. What is your evaluation of the implementation? 4. What is your evaluation of the documentation? 5. What is your evaluation of the potential usefulness of the library? 6. Did you try to use the library? With what compiler? Did you have any problems? 7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
On 2015-08-15 10:01, Bjorn Reese wrote:
A quick reminder that the formal review ends on Sunday (August 16th.) Oh my, time flies...
Please consider submitting a review where the following questions are answered:
1. Should Boost.Http be accepted into Boost? Please state all conditions for acceptance explicity. I did expect a library that
* offers both client and server side * offers a high level interface that allows me to get started very quickly, without having to worry about the gory details * offers tons of ways to tune, in case I feel the need to do that * supports HTTP/1 through HTTP/2 I got * server side only * the low level interface only * no HTTP/2 * probably lots of tuning options (but see documentation) I can live with not having a client side at the beginning, but if the current library should be accepted, a full review of the client part would be required. Having no HTTP/2 support is a surprise, as I consider boost to be cutting edge. Having no high level interface is a show stopper for me. I won't try to work my way through the details if I need what feels like a hundred lines of boiler plate before even receiving the first byte. And then I would have to care about stuff like: "You should send a 100-continue to ask for the rest of the body if required." If it is required, it is required. Why do I have to check for it myself? The library should take care of that (unless I tell it not to, because I want to do my own magic). Similar with chunking. If I send chunks and I should not, can't the library take care of it by default (i.e. by buffering the data)? Or this: self->socket.async_write_response(200, string_ref("OK"), reply, yield); Admittedly, not horribly much to write, but since I am using an HTTP library, I would have expected convenience functions and enums that make sure I do not mess stuff up (e.g. by sending 404/OK). Reading the documentation did not exactly encourage me to play with the library. Looked to me like I need to know my way around HTTP very, very well to not run into traps all the time. Thus, I admit that I really haven't invested that much time, but as of now, I vote for No, the library should not be included.
2. What is your evaluation of the design?
3. What is your evaluation of the implementation? I haven't really looked into the code.
There was some discussion about the library being C++11 only. I do not see that as a problem for a new library. I can totally understand if a library author these days does not want to spend time in supporting older compilers. While this will certainly limit the number of users in the beginning, I believe that it is the right strategy in the long run (and it saves time and nerves of the author).
4. What is your evaluation of the documentation?
The last time I did something with Boost.Asio was about five years ago. That might be why I find it hard to read, but I doubt it. Maybe there isn't much to say, since everything is so low level that it is basically Boost.Asio, but the long reference list makes this doubtful as well. It is a tutorial with basically one example. The front page has hints to a lightweight HTTP Server and a file server (er, what?), but I did not find any documentation for those. I cannot really tell what else is all missing, but the documentation seems to be rather incomplete.
5. What is your evaluation of the potential usefulness of the library?
Potentially very useful. It just does not seem to be there yet.
6. Did you try to use the library? With what compiler? Did you have any problems?
No.
7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
About an hour of reading, then about 2 hours of going back and forth between this mail and the documentation. I would really like to see an HTTP library for Boost. I just pushed a library for text templates (e.g. HTML) to github. I would like to combine that with an HTTP server for some projects. It could very well be based on Boost.HTTP eventually. But I do not think it is ready yet. More convenience for the casual users and certainly more documentation (and probably a different tutorial) for such users will certainly help a lot. Since I did no dig very deep, I can't tell if that would be sufficient, though. Best, Roland
2015-08-15 9:28 GMT-03:00 Roland Bock
"You should send a 100-continue to ask for the rest of the body if required."
If it is required, it is required. Why do I have to check for it myself? The library should take care of that (unless I tell it not to, because I want to do my own magic).
Fair. I could add some magic to this. Similar with chunking. If I send chunks and I
should not, can't the library take care of it by default (i.e. by buffering the data)?
The library could fallback to buffering if chunk is unsupported, but it is a mistake (as such, an error is raised[1]). You'll segfault your application if you try to stream a live video stream and the library implicitly starts to buffer the response thanks to inability of the underlying communication channel in provide native stream. This API would be highly unsafe. It's not worthy. If you don't need native stream, just send the whole message at once. It's extremely simple. I don't see a point here. If your message doesn't have an infinite size, you'll just write the body and schedule the message to be written. No big deal. Or this:
self->socket.async_write_response(200, string_ref("OK"), reply, yield);
Admittedly, not horribly much to write, but since I am using an HTTP library, I would have expected convenience functions and enums that make sure I do not mess stuff up (e.g. by sending 404/OK).
There is already a function to make this task safer: https://boostgsoc14.github.io/boost.http/reference/async_write_response.html It just you happens to write more than just "OK". User preferences, both styles are supported. I cannot really tell what else is all missing, but the documentation
seems to be rather incomplete.
Actually, the documentation is quite exhaustive. However, it lacks examples (i.e. it says what/how/why, but it won't give an example) and a more teaching approach (I need to draft a plan on how I can improve this (lack of) skill of mine or find contributors). [1] native_stream_unsupported, https://boostgsoc14.github.io/boost.http/reference/http_errc.html -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
2015-08-15 9:28 GMT-03:00 Roland Bock
: "You should send a 100-continue to ask for the rest of the body if required."
If it is required, it is required. Why do I have to check for it myself? The library should take care of that (unless I tell it not to, because I want to do my own magic).
Fair. I could add some magic to this.
Similar with chunking. If I send chunks and I
should not, can't the library take care of it by default (i.e. by buffering the data)?
The library could fallback to buffering if chunk is unsupported, but it is a mistake (as such, an error is raised[1]). You'll segfault your application if you try to stream a live video stream and the library implicitly starts to buffer the response thanks to inability of the underlying communication channel in provide native stream. This API would be highly unsafe. It's not worthy. If you don't need native stream, just send the whole message at once. It's extremely simple. I don't see a point here. If your message doesn't have an infinite size, you'll just write the body and schedule the message to be written. No big deal.
Or this:
self->socket.async_write_response(200, string_ref("OK"), reply, yield);
Admittedly, not horribly much to write, but since I am using an HTTP library, I would have expected convenience functions and enums that make sure I do not mess stuff up (e.g. by sending 404/OK).
There is already a function to make this task safer: https://boostgsoc14.github.io/boost.http/reference/async_write_response.html
It just you happens to write more than just "OK". User preferences, both styles are supported.
I cannot really tell what else is all missing, but the documentation
seems to be rather incomplete.
Actually, the documentation is quite exhaustive. However, it lacks examples (i.e. it says what/how/why, but it won't give an example) and a more teaching approach (I need to draft a plan on how I can improve this (lack of) skill of mine or find contributors). The reference may be exhaustive, but that can't be a replacement for the documentation. It is a support tool. I won't learn how to use the
On 2015-08-15 17:27, Vinícius dos Santos Oliveira wrote: library by reading the reference from a through z. More examples are required. A few short ones especially. How to write a concurrent not-too-bad-performing http server in less than 20 lines of code. Something like that. Something that makes me want to use it because it is painless. Some more advanced examples, e.g. including TLS with client/server certificates would help.
I'm out of time to actually try the library, so will give my review based on just the study of the documentation/mailing list comments.
1. Should Boost.Http be accepted into Boost? Please state all conditions for acceptance explicitly.
No, sorry.
2. What is your evaluation of the design? 3. What is your evaluation of the implementation? 4. What is your evaluation of the documentation? 5. What is your evaluation of the potential usefulness of the library?
From what I can tell, the design is an ambitious attempt to be all
The documentation did not allow me to tell if this is a weak library, or a good library that is being marketed badly. It contains a single example which is very long and off-putting. The initial page gives a list of features, and the design rationale repeats them, but not how to use any of them. The first example in the documentation should be no more than 10 lines. Then there must be examples that show: SSL (using a self-signed certificate), a file server, a routing application with 2-3 routers doing distinct things and an example of streaming data. things to all people. IMHO this is a fatal mistake. Naming and Scope: It should be called Boost.HttpServer; and then there should be another library called Boost.HttpClient. Reasons: 1. They will share very little public API code 2. The majority of projects that need an http server will not also need an http client; the huge majority of projects that need an http client will not need an http server. (The 1% that need both can just include both libraries.) 3. There is not actually a great amount of implementation code they share. 4. Http is a protocol. Libraries shouldn't map to protocols, they should map to the work people need to do. API Level: What C++ badly needs is a high-level, easy to use http server library. It needs a high-level http client library even more badly. The proposed library looks like some building blocks towards this. I suggest it either stay as a github project that an Boost.HttpServer can build upon, or be merged into asio as an extension. Here is the kind of http server api I was hoping to see up for review (this is the short motivating example I expected to see as the first thing in the tutorial): (...moved to [1]) My opinion on some other topics the review has brought up: Header-only: Yes, important. Pre-C++11 support: No need. C++11 is more pleasant, and building in reliance on lambda or any other C++11 feature is acceptable if it makes the design nicer. I could even accept it being C++14-only if there was a good reason. Asio Integration: No need. I have used boost::asio on some projects. I do not enjoy using it, it is easy to get things wrong, code tends to become long and complicated and/or it is under-documented. (While I wait for my ideal C++ http-server I will now use posix socket's C functions directly. And while waiting for my ideal C++ http-client implementation, I am using curl.) In other words, if it is built on top of asio I want that as an implementation detail I don't have to care about. Http/2: Not essential at this stage. However it has to be clear that the design and API supports moving to it transparently. As that may not be trivial, I think a library offered for review does have to show support for http/2. Streaming: I would like to see examples of sending a chunked response, and of sending a streaming (SSE or old-fashioned comet) response that never closes. This may reveal bugs or design flaws. Routing: A built-in router should be part of the version for review. It should be a distinct component so that alternatives can be used, but `boost::httpserver::server` should be using a "pleases-most-people-most-of-the-time" implementation. If routing is not included, everyone will end up rolling their own, and that is bad. Benchmarks: I would like to see CPU and memory usage benchmarks; ideally on reference hardware such as an Amazon EC2 instance. Realistic examples such as: a simple file server, a file-server that is doing substitution/modification of the files being read, and a chat server. This is because artificial benchmarks often encourage making the design harder to use for the sake of a few microseconds. I'd rather have a server, doing real work, that can support 499 clients/second than one that can support 500 clients/second but requires me to write twice as much code.
6. Did you try to use the library? With what compiler? Did you have any problems?
No.
7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Maybe 10 hours reading the documentation, following the discussion, and forming my thoughts here. I only glanced at the code on github (mainly in the hope of finding more examples or documentation).
8. Are you knowledgeable about the problem domain?
Reasonably so. I've written socket clients and socket servers in C++,
C#, PHP and Node.js. I have used asio, and looked at various C++ http
libraries (usually ending up rejecting them all). I have a lot of
experience with LAMP stacks, and with http scaling issues. For an
http server API my favourite design is node.js's http module (though it
lacks routing). For an http client I like the power and simplicity of
PHP's file_get_contents().
Darren
--
Darren Cook, Software Researcher/Developer
My new book: Data Push Apps with HTML5 SSE
Published by O'Reilly: (ask me for a discount code!)
http://shop.oreilly.com/product/0636920030928.do
Also on Amazon and at all good booksellers!
[1]:
#include
2015-08-15 16:18 GMT-03:00 Darren Cook
I'm out of time to actually try the library, so will give my review based on just the study of the documentation/mailing list comments.
Thanks for your review, Darren. I found it valuable. -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On 7 Aug 2015 at 9:08, Bjorn Reese wrote:
Please answer the following questions:
1. Should Boost.Http be accepted into Boost? Please state all conditions for acceptance explicity.
Yes, with these conditions: 1. It should not be called Boost.Http. Users will expect a high-level *client* API in a library called Boost.Http. I'd suggest: Boost.AsyncHttpServer Boost.ASIOHttpServer Boost.HttpServer 2. I think a client HTTP library is a lot more useful to Boost users than a server HTTP library. I think it needs a client HTTP library before acceptance. I would even suggest you drop the server library in favour of the client library - far more people need and want a client library over a server library. 3. It needs HTTP 2.0 support. 4. It needs to be socket input fuzz tested on a weekly or more basis. 5. The documentation needs a very great deal of additional work. See below. 6. That if conditionally accepted, the library must reenter the peer review queue and undergo a second full community review. The latter condition could be interpreted as me recommending rejection of Http at this point. I wouldn't go as far as that - I like the library, I like the design, and I think it could be a great addition to Boost in the future.
2. What is your evaluation of the design?
Unlike some of the other reviewers, I'm relatively fond of the design given it's a HTTP server API not a client API. I would personally speaking say it involves too many publicly exposed moving parts for my own taste. I really don't want to know about the moving parts - I just want to serve some HTTP in less than 50 lines of code. I don't care about performance until after I am serving some HTTP, and I don't want to think about internals or specifics or even async until I benchmark a performance problem. If I were designing something like a HTTP client library, I personally would start with a C++ edition of the Python Requests library API (http://www.python-requests.org/en/latest/api/) which I've enjoyed using in Python and go from there.
3. What is your evaluation of the implementation?
The quality of implementation is pretty high. I felt a lot more work needed to be done on testing.
4. What is your evaluation of the documentation?
The documentation needs a very great deal of additional work. Particularly regarding the tutorial and quickstart. Right now it's nowhere close to Boost ready.
5. What is your evaluation of the potential usefulness of the library?
I'm not convinced that a HTTP server only library is as useful to as many as a HTTP client library. I think a HTTP client library with a high level API would be very warmly received at Boost. Even better if it had good performance and internal layers which could be unpicked for custom behaviour, but to be honest neither of those is necessary: all 95% of users want is to fetch a HTTP resource and post form content. Only a tiny minority are interested in serving HTTP. For that tiny minority interested in serving HTTP, I am unconvinced they would consider the steep learning curve imposed by proposed Boost.Http as worth it when compared to writing some temporary hack code which implements a quick and dirty HTTP server on ASIO. The time spent learning proposed Http I reckon is about the same as writing a quick and dirty hack, and the latter is much more fun.
6. Did you try to use the library? With what compiler? Did you have any problems?
No.
7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I have been following the library since its inception during GSoC.
8. Are you knowledgeable about the problem domain?
Relatively knowledgeable. I have written a hacky quick HTTP server in ASIO before. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Fri, Aug 7, 2015 at 3:08 AM, Bjorn Reese
Dear Boost and ASIO communities.
The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th.
Boost.Http is designed for various forms of modern HTTP interaction, from normal HTTP request, over HTTP chunking and pipelining, to upgrading to other web protocols like WebSocket. This library builds on top of Boost.ASIO, and follows the threading model of ASIO.
The two basic building-blocks are http::socket, which is socket that talks HTTP, and http::message with contains HTTP meta-data and body information. You can use these building-blocks to build a HTTP server that fits your exact needs; for instance, an embedded HTTP server for a ReST API. Boost.Http comes with a light-weight HTTP server and a static file server.
Currently, Boost.Http is limited to server-side interaction, but the design principles used extends to client-side as well.
Boost.Http was originally developed as part of GSoC 2014 and Vinícius has continued to develop and improve the library since then.
The documentation can be found here:
http://boostgsoc14.github.io/boost.http/
The source code can be found here:
https://github.com/BoostGSoC14/boost.http
The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.)
Please answer the following questions:
1. Should Boost.Http be accepted into Boost? Please state all conditions for acceptance explicity.
I do not think Boost.Http should be accepted in its current state. My main reasons (read portions below for expanded comments/thoughts): (1) Incomplete design - There is no built-in mechanism for writing out client headers, which means there is no client support. (2) Unproven design - A low-level abstraction for a "HttpSocket" is provided, but only two higher-order methods for file serving are provided. Why aren't there higher-order functions for more common use cases? (3) More considerations for design choices (or documentation wording) should be considered - All data reads / writes for the message body require a copy to be made. (4) More time is needed to think about the design patterns (this follows from a lack of (2)) - why is `async_write_response_continue` a requirement for the http::ServerSocket concept? (5) Documentation can be better organized and lacks critical information in a few areas. 2. What is your evaluation of the design?
- There is no built-in mechanism for writing out client headers, which means there is no client support. The provided rationale is that it took a long-time to design the server portion, and the client side will take equally as long. I don't see this as a valid argument; several current library authors have indicated that providing a "boost-ready" library is an incredible amount of work. I would expect a boost::http library to be more feature complete before being accepted. - An abstraction for a "HttpSocket" is provided, but only a few higher-order functions are provided (for file serving). The current lower-level abstractions _should_ allow for useful utilities to be written, but I would like to see more functionality implemented before boost acceptance, so the lower-level design is more "proven". - The http::ServerSocket concept defines methods for writing response headers, writing the body, writing trailers, and writing an entire response message at once. This concept provides everything necessary for writing a valid HTTP response. So why is `async_write_response_continue` mandated in the http::ServerSocket concept when a free function can be implemented, that uses the other concept functions? If the reasoning is performance (which appears to be the reason) - why isn't a more performant method (probably some form of pre-generation) of sending messages added? This would prevent the socket concept from continually absorbing additional special case messages as higher order functions are developed, and allow for clients to pre-generate error responses. - `async_read_request`, and `async_write_response_metadata` in the http::ServerSocket concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary for this function. - `async_write` and `async_read_some` in the http::Socket concept requires an entire http::Message concept as an argument, when only the SequenceContainer concept should be necessary for this function. - All data reads / writes for the message body require a copy to be made, because memory from a SequenceContainer concept cannot be given to ASIO/OS directly (std::liststd::uint8_t is a SequenceContainer). The message body should likely be restricted to the C++1z ContiguousContainer concept instead. I think users would prefer speed here more than flexibility. Its also easier to relax requirements after release than the opposite. - Ideally the read / write functions for the payload would have the same requirements as an asio buffer. Requiring a container prevents memory locations from other sources (such as directly from a streambuf), and prevents scatter gather i/o. I'd say this is less critical, as its something most users are unlikely to use. - `async_read_trailers` and `async_writer_trailers` in the http::Socket concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary. - I mentioned this before; client and server specific header writing/reading is a shame due to the overwhelming similarities. My earlier suggestions were criticized for being stringly typed (or having more states), but I think this was a somewhat weak criticism since the type system can be used more effectively. `async_write_header(http::response{200, "OK"}, ...)` or `async_write_header(http::request{http::request::GET, "/"}, ...)`. - It might be worth eventually relaxing the requirements for the key/values in the AssociativeMap used by the field parsing functions to a subset of string functions. begin(), end(), size(), empty(), push_back(), pop_back(), operator<(), a hash implementation, and contigous data. This way a quick and dirty small string optimization could be written for field handling. - What is the purpose of the standalone `async_write_response` and `async_write_response_metadata` functions? They only forward to functions with the same name in the http::ServerSocket concept, is this cruft? - I like that all of the asynchronous callbacks take consistent parameters. asio::coroutine works nicely.
3. What is your evaluation of the implementation?
Overall the quality is good. I didn't go read the code thoroughly, but I did notice a few things - - The current `async_write_response` implementation assumes contiguous elements on the SequenceContainer concept, which is incorrect. - A std::ostringstream is used, when spirit::karma would be faster and have less memory allocations (minor). - The current `async_read_some` implementation has an error code path that can initiate a write. This is not documented anywhere, and effectively issuing an `async_read_some` command blocks both the send and receive channels of the underlying ASIO stream. This problem is likely to occur in a few other spots, but I didn't check. - There are some error code paths in async operations that modify the entire message object (clear() is invoked on all parts of the message). Seems unnecessary, and is at least undocumented. 4. What is your evaluation of the documentation?
Needs better organization, and someone to edit the rationale section in particular. - The reference section is organized strangely. Every class, concept, function, and file is listed under the same header, and then its broken into sections at the bottom. I didn't notice the bottom portion at first, and it should replace the top portion that lacks organization. - How do the various write functions manipulate the headers? What fields are added, if any? This is partially mentioned at the bottom of the http::ServerSocket page, but I saw "content-length: 22" added to my message, and this was never explicitly stated (although obviously assumed). This should likely be explicitly specified in the concept itself, AND the location should be specified (i.e. these fields are explicitly appended). Should also document that some fields, such as content-length cannot be listed twice according to the specification, while comma de-limited fields can be listed multiple times as a valid form of appension. I.e. "content-encoding: gzip, sdch\r\n" and "content-encoding: gzip\r\ncontent-encoding: sdch\r\n" indicate equivalent content encodings applied to the data. Should probably mention that Transfer-Encoding is a comma delimited list too. - The `key_type` and `mapped_type` for the headers portion of the message::Concept indicate that they must meet the "String concept". I don't recall seeing this concept being defined in C++ or boost, where does it come from? If its from C++1z, it might be to merge the behavior of string_ref and basic_string, so that concept would be unlikely to require storage like a container. Might want to re-think the concept requirements here, or state that only a conforming std::basic_string implementation is acceptable for now. - Mentioned previously - the documentation for basic_socket should state when a function is using the read or write portion of the stream. Currently some functions use both unexpectedly, such as async_read_some.
5. What is your evaluation of the potential usefulness of the library?
This library has multiple potentially use cases, especially if a good low-level design is provided. Additional higher-level functions would be nice, because many (perhaps most) users have similar requirements, don't need extreme performance, and just want something simple and relatively efficient. Hopefully work will be continued on this library, regardless of the outcome of this review process. 6. Did you try to use the library? With what compiler? Did you have
any problems?
I ran all of the tests on OSX, which had no complaints. I also wrote a quick asio::coroutine implementation, and viewed the data passing by in wireshark for good measure.
7. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I spent a good amount of time reading the documentation, and implementation. A little less testing / running code.
8. Are you knowledgeable about the problem domain?
I have fairly extensive knowledge of protocols, and experience providing implementations for protocols (including HTTP). I also have a reasonable amount of ASIO experience. Lee
Thanks for your review Lee, your comments are surely helpful for me, as I'm
going to do several improvements based on them.
2015-08-17 2:01 GMT-03:00 Lee Clagett
So why is `async_write_response_continue` mandated in the http::ServerSocket concept when a free function can be implemented, that uses the other concept functions? If the reasoning is performance (which appears to be the reason) - why isn't a more performant method (probably some form of pre-generation) of sending messages added? This would prevent the socket concept from continually absorbing additional special case messages as higher order functions are developed, and allow for clients to pre-generate error responses.
`async_write_response_continue` is a different concept, it cannot be composed in terms of the other operations. It has a different semantic. It changes the read_state status. - `async_read_request`, and `async_write_response_metadata` in the
http::ServerSocket concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary for this function.
- `async_write` and `async_read_some` in the http::Socket concept requires an entire http::Message concept as an argument, when only the SequenceContainer concept should be necessary for this function.
Just passing the message object is less error-prone (e.g. should I pass headers() or trailers()?). But yes, I could do better (decrease requirements and add a few helper functions/types). - All data reads / writes for the message body require a copy to be made,
because memory from a SequenceContainer concept cannot be given to ASIO/OS directly (std::liststd::uint8_t is a SequenceContainer). The message body should likely be restricted to the C++1z ContiguousContainer concept instead. I think users would prefer speed here more than flexibility. Its also easier to relax requirements after release than the opposite.
std::vector is used by default. Not sure how helpful it is to restrict the user if he is willing to pay. A parser embedded in the message object would have trouble implementing the ContiguousContainer concept when it comes to chunked entities. I agree that's easier to relax requirements after release than the opposite. - Ideally the read / write functions for the payload would have the same
requirements as an asio buffer. Requiring a container prevents memory locations from other sources (such as directly from a streambuf), and prevents scatter gather i/o. I'd say this is less critical, as its something most users are unlikely to use.
It may kill some use cases. If some type is more performant for the user, it should be used. The default should satisfy most of the users wish. - `async_read_trailers` and `async_writer_trailers` in the http::Socket
concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary.
Just passing the message object is less error-prone (e.g. should I pass headers() or trailers()?). But yes, I could do better (decrease requirements and add a few helper functions/types). - I mentioned this before; client and server specific header
writing/reading is a shame due to the overwhelming similarities. My earlier suggestions were criticized for being stringly typed (or having more states), but I think this was a somewhat weak criticism since the type system can be used more effectively. `async_write_header(http::response{200, "OK"}, ...)` or `async_write_header(http::request{http::request::GET, "/"}, ...)`.
This design would have much more implicit behaviour and it'd lead to more surprises. A ResponseBuilder or alike could be done, separately, in case you don't mind the surprises. - What is the purpose of the standalone `async_write_response` and
`async_write_response_metadata` functions? They only forward to functions with the same name in the http::ServerSocket concept, is this cruft?
They fill the reason phrase for you based on the status code. - The current `async_read_some` implementation has an error code path that
can initiate a write. This is not documented anywhere, and effectively issuing an `async_read_some` command blocks both the send and receive channels of the underlying ASIO stream. This problem is likely to occur in a few other spots, but I didn't check.
The ServerSocket concept gives guarantees about a producer to HTTP messages (and you, as the user, writes the HTTP consumer). A channel to exchange HTTP messages somehow. There were no guarantees about exposing the implementation detail errors to the user. Imagine a ZeroMQ-based ServerSocket that cannot send "pings" because it was not documented in the general concept. The general ServerSocket concept doesn't restrict the freedom from HTTP "providers" too much. Anyway, this is a serious documentation error within the library and I need to fix it. -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On Tue, Aug 25, 2015 at 1:43 AM, Vinícius dos Santos Oliveira < vini.ipsmaker@gmail.com> wrote:
Thanks for your review Lee, your comments are surely helpful for me, as I'm going to do several improvements based on them.
2015-08-17 2:01 GMT-03:00 Lee Clagett
: So why is `async_write_response_continue` mandated in the http::ServerSocket concept when a free function can be implemented, that uses the other concept functions? If the reasoning is performance (which appears to be the reason) - why isn't a more performant method (probably some form of pre-generation) of sending messages added? This would prevent the socket concept from continually absorbing additional special case messages as higher order functions are developed, and allow for clients to pre-generate error responses.
`async_write_response_continue` is a different concept, it cannot be composed in terms of the other operations. It has a different semantic. It changes the read_state status.
Did you mean write_state (if not, update documentation)? And yes, I missed this state transition when making this suggestion. The purpose of the state appears to be (and please correct me otherwise) is to prevent consecutive 100 continue responses from being written. I don't think its worth the extra state transition because the read calls do not put the write_state into a special state that forces a 100 continue OR error response. So a user of the library already has to know to check for expect-100's in the request, at which point it seems reasonable to expect the user to NOT write consecutive 100-continues. My suggestion would be to either drop the state entirely (allowing the user to "accidentally" write consecutive 100-continues) OR allow a write_state change from a server header read into a expect-100-continue-response state. The latter change may seem unpalatable, but this implementation is already "locking" the write side of the channel during server reads to hide certain error responses from users (bad http version, etc.).
- `async_read_request`, and `async_write_response_metadata` in the
http::ServerSocket concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary for this function.
- `async_write` and `async_read_some` in the http::Socket concept requires an entire http::Message concept as an argument, when only the SequenceContainer concept should be necessary for this function.
Just passing the message object is less error-prone (e.g. should I pass headers() or trailers()?). But yes, I could do better (decrease requirements and add a few helper functions/types).
because memory from a SequenceContainer concept cannot be given to ASIO/OS directly (std::liststd::uint8_t is a SequenceContainer). The message
- All data reads / writes for the message body require a copy to be made, body
should likely be restricted to the C++1z ContiguousContainer concept instead. I think users would prefer speed here more than flexibility. Its also easier to relax requirements after release than the opposite.
std::vector is used by default. Not sure how helpful it is to restrict the user if he is willing to pay. A parser embedded in the message object would have trouble implementing the ContiguousContainer concept when it comes to chunked entities.
I agree that's easier to relax requirements after release than the opposite.
A trait dispatched system could be provided if someone really wanted SequenceContainer support. But its just too odd - std::liststd::uint8_t and other node based containers would be really low performing. Again, the ASIO method is more flexible because it allows for contiguous chunks of memory, or a range of contiguous chunks of memory to be provided. So a std::liststd::uint8_t should be supported by the ASIO interface actually (oh my!), but obviously a range of 1-byte chunks is also likely poor performing.
- Ideally the read / write functions for the payload would have the same
requirements as an asio buffer. Requiring a container prevents memory locations from other sources (such as directly from a streambuf), and prevents scatter gather i/o. I'd say this is less critical, as its something most users are unlikely to use.
It may kill some use cases. If some type is more performant for the user, it should be used. The default should satisfy most of the users wish.
What use cases does a container provide over an ASIO buffer? The only one I can recall being mentioned is a wire & transport implementation that sends/receives data as messages instead of a stream. I suspect the flexibility of the ASIO buffer would appeal to more people than the ability to read ZeroMQ messages with a slight efficiency advantage. I obviously can't back this claim with any statistics, so I won't push much further on this topic. Are there other advantages or special use cases for the container-based system? Have you considered standalone functions for writing/reading standard HTTP/1.x chunked data directly from an ASIO stream? There might be ways to allow effective usage of the native handle for advanced users (thus keeping the default container implementation).
- `async_read_trailers` and `async_writer_trailers` in the http::Socket
concept require an entire http::Message concept as an argument, when only the AssociativeContainer concept should be necessary.
Just passing the message object is less error-prone (e.g. should I pass headers() or trailers()?). But yes, I could do better (decrease requirements and add a few helper functions/types).
- I mentioned this before; client and server specific header
writing/reading is a shame due to the overwhelming similarities. My earlier suggestions were criticized for being stringly typed (or having more states), but I think this was a somewhat weak criticism since the type system can be used more effectively. `async_write_header(http::response{200, "OK"}, ...)` or `async_write_header(http::request{http::request::GET, "/"}, ...)`.
This design would have much more implicit behaviour and it'd lead to more surprises. A ResponseBuilder or alike could be done, separately, in case you don't mind the surprises.
What do you mean by implicit behavior? And what surprises? The name of the type indicates the action being taken ... ?
- What is the purpose of the standalone `async_write_response` and
`async_write_response_metadata` functions? They only forward to functions with the same name in the http::ServerSocket concept, is this cruft?
They fill the reason phrase for you based on the status code.
- The current `async_read_some` implementation has an error code path that
can initiate a write. This is not documented anywhere, and effectively issuing an `async_read_some` command blocks both the send and receive channels of the underlying ASIO stream. This problem is likely to occur in a few other spots, but I didn't check.
The ServerSocket concept gives guarantees about a producer to HTTP messages (and you, as the user, writes the HTTP consumer). A channel to exchange HTTP messages somehow. There were no guarantees about exposing the implementation detail errors to the user.
Imagine a ZeroMQ-based ServerSocket that cannot send "pings" because it was not documented in the general concept. The general ServerSocket concept doesn't restrict the freedom from HTTP "providers" too much.
The general concept needs to stipulate whether reads or writes can be made by the client after initiating an asynchronous operation. This is the most important thing that must be documented, because the current implementation cannot safely accept a write operation when some (all?) of the read operations are still in progress. I think its acceptable for an implementation to inject control messages under such restrictions, as long as the user can still issue their own write when the specification allows for it. Such implementations _may_ have to indicate the control message behavior when documenting access to the "native handle" (and I suspect "may" is going to be "always" in practice), in case a user tries to do some raw writing. Or maybe the documentation just says "never do raw read/writes" with the native handle. Anyway, this is a serious documentation error within the library and I need
to fix it.
This isn't just a serious documentation error, its a design problem for delivering lower-latency responses. Since the read call can issue an automatic error message on the write channel, users of the library cannot begin reading the next request (which is likely already in a kernel buffer if the client is pipelining requests) until the prior response has finished sending. If the errors were not automatically written by the implementation, a user could began reading the next request (potentially even preparing DB calls, etc.) while waiting for the response of the prior request to complete. I do not think this is a fatal issue, but it does make me question the low-level design goals. The design/implementation seems to be halfway between a constricting easy-to-use high-level design and a performant low-level design. It seems reasonable to expect users of the low-level interface to have more understanding of HTTP, while the high-level design provides a better "safety net" for users. The low-level users would still benefit from an HTTP system that reads/writes headers into/from an associative container, handles receiving/sending chunked data automatically, etc., which is convenient. Lee
2015-08-25 18:44 GMT-03:00 Lee Clagett
What use cases does a container provide over an ASIO buffer?
The user may implement a Message concept that will just drop the body entirely. To be fair, this violates the concept I used, but that was one of the expectations I had when I was designing the class. I should update the requirements to better reflect the state... Are there other advantages or special use cases for the container-based
system?
It separates the responsibility between pass HTTP messages and produce HTTP messages more easily, which is great for multiple backends support. There is the embedded server backend, for instance, which produce HTTP messages and feed them to the user, which consumes them. Have you considered standalone functions for writing/reading standard
HTTP/1.x chunked data directly from an ASIO stream?
I have thought about it, but I never considered this approach, because I'm aiming for multiple backends support. But I thought about specializing the concepts in such a way that you could move the HTTP parser to the HTTP message object (that's why headers and others are member-functions, not member variables, so you can have a large blob of memory that has all the info that is shared by all functions). There might be ways to allow effective usage of the native handle for
advanced users (thus keeping the default container implementation).
From the feedback I had, advanced users fall into three categories:
- They don't care about multiple backends, so an HTTP parser is the only abstraction they will approve. - They don't know HTTP well enough and had suggested me to *drop* HTTP features, so the design would *evolve*, which is very contradictory for an *HTTP library*. - They had awesome ideas and gave me excellent feedback that I'll use to improve the library design. Anyway, for the "allow effective usage of native handle", I'll just provide an HTTP parser, as they probably don't care about multiple backend. - I mentioned this before; client and server specific header
writing/reading is a shame due to the overwhelming similarities. My earlier suggestions were criticized for being stringly typed (or having more states), but I think this was a somewhat weak criticism since the type system can be used more effectively. `async_write_header(http::response{200, "OK"}, ...)` or `async_write_header(http::request{http::request::GET, "/"}, ...)`.
This design would have much more implicit behaviour and it'd lead to more surprises. A ResponseBuilder or alike could be done, separately, in case you don't mind the surprises.
What do you mean by implicit behavior? And what surprises? The name of the type indicates the action being taken ... ?
Individually write each header means you either is buffering all headers and waiting til the first chunk of body is issued or you're sending them as you receive them. This is already implicit behaviour, but there is more. Details about the body delivery must be present in the header section of the message, but the write_header API don't expose user intent about whether he is willing to create an atomic message or a chunked message and workarounds about this bad API must be done to avoid errors like the header section not being sent while the body isn't issued (which is problematic in case of an event system and could be solved with a flush_headers), a 0-sized chunked message being issued for a 204 reply, and maybe more. There are many more code paths possible that needed to be documented and are tricky to use correctly. If a user want the write_header API, I'd be on top of current API as a separate abstraction (ResponseBuilder?), as it'd be more clear what would happen and will only put risk on users willing to take it. - The current `async_read_some` implementation has an error code path that
can initiate a write. This is not documented anywhere, and effectively issuing an `async_read_some` command blocks both the send and receive channels of the underlying ASIO stream. This problem is likely to occur in a few other spots, but I didn't check.
The ServerSocket concept gives guarantees about a producer to HTTP messages (and you, as the user, writes the HTTP consumer). A channel to exchange HTTP messages somehow. There were no guarantees about exposing the implementation detail errors to the user.
Imagine a ZeroMQ-based ServerSocket that cannot send "pings" because it was not documented in the general concept. The general ServerSocket concept doesn't restrict the freedom from HTTP "providers" too much.
The general concept needs to stipulate whether reads or writes can be made by the client after initiating an asynchronous operation. This is the most important thing that must be documented, because the current implementation cannot safely accept a write operation when some (all?) of the read operations are still in progress. I think its acceptable for an implementation to inject control messages under such restrictions, as long as the user can still issue their own write when the specification allows for it. Such implementations _may_ have to indicate the control message behavior when documenting access to the "native handle" (and I suspect "may" is going to be "always" in practice), in case a user tries to do some raw writing. Or maybe the documentation just says "never do raw read/writes" with the native handle.
A model of the general concept may not even use a socket. Therefore, the general concept cannot assume a read will be issued at all and no further assumptions about them. This documentation belongs to specific models. And the write code path you mention is like when the socket has become useless because non-conformant HTTP traffic was detected. However, I agree that documentation needs improvement here too. Anyway, this is a serious documentation error within the library and I need
to fix it.
This isn't just a serious documentation error, its a design problem for delivering lower-latency responses. Since the read call can issue an automatic error message on the write channel, users of the library cannot begin reading the next request (which is likely already in a kernel buffer if the client is pipelining requests) until the prior response has finished sending. If the errors were not automatically written by the implementation, a user could began reading the next request (potentially even preparing DB calls, etc.) while waiting for the response of the prior request to complete.
Okay. I do not think this is a fatal issue, but it does make me question the
low-level design goals. The design/implementation seems to be halfway between a constricting easy-to-use high-level design and a performant low-level design. It seems reasonable to expect users of the low-level interface to have more understanding of HTTP, while the high-level design provides a better "safety net" for users. The low-level users would still benefit from an HTTP system that reads/writes headers into/from an associative container, handles receiving/sending chunked data automatically, etc., which is convenient.
I'm actually aiming for high-performance high-level design and I expect to see these same abstractions involved in a higher level layer. Anyway, it seems I have lots of changes to do in the library. -- Vinícius dos Santos Oliveira https://about.me/vinipsmaker
On 07-Aug-15 10:08 AM, Bjorn Reese wrote:
Dear Boost and ASIO communities.
The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th.
The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.)
I think that per http://www.boost.org/community/reviews.html the above should have blocked formal review. If a library is reviewed, it normally means the author believes it's to be ready for Boost, which certainly is not the case if one cannot run the test using the current tools. - Volodya
On 08/17/2015 09:19 AM, Vladimir Prus wrote:
On 07-Aug-15 10:08 AM, Bjorn Reese wrote:
The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.)
I think that per http://www.boost.org/community/reviews.html the above should have blocked formal review. If a library is reviewed, it normally means the author believes it's to be ready for Boost, which certainly is not the case if one cannot run the test using the current tools.
That is a fair point to raise. I did not see the lack of Boost.Build as an impediment to the review for various reasons: 1. This is not the first library without Boost.Build support during review (e.g. Boost.Hana.) 2. The current CMake support is easier to use for the reviewers who want to take the library for a test-drive. Using Boost.Build would require that they copy Boost.Http into their Boost source tree. I find it unfortunate to force reviewers to "pollute" their Boost source tree with candidate libraries. 3. The Boost.Http documentation is already build using Boost.Build, so the transition of the remining source code should be minimal.
2. The current CMake support is easier to use for the reviewers who want to take the library for a test-drive. Using Boost.Build would require that they copy Boost.Http into their Boost source tree. I find it unfortunate to force reviewers to "pollute" their Boost source tree with candidate libraries.
+1 - strongly agree. -- This message is subject to the CSIR's copyright terms and conditions, e-mail legal notice, and implemented Open Document Format (ODF) standard. The full disclaimer details can be found at http://www.csir.co.za/disclaimer.html. This message has been scanned for viruses and dangerous content by MailScanner, and is believed to be clean. Please consider the environment before printing this email.
On 8/17/15 12:19 AM, Vladimir Prus wrote:
On 07-Aug-15 10:08 AM, Bjorn Reese wrote:
Dear Boost and ASIO communities.
The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th.
The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.)
I think that per http://www.boost.org/community/reviews.html the above should have blocked formal review. If a library is reviewed, it normally means the author believes it's to be ready for Boost, which certainly is not the case if one cannot run the test using the current tools.
I have to disagree with this. a) The above cited link doesn't specify support for boost build as a requirement. b) No one complained during the review that lack of support for boost build inhibited them from reviewing the submission. c) The fact that support for boost build may be required for library acceptance doesn't imply that it should be necessary for review. Robert Ramey
On 17-Aug-15 5:38 PM, Robert Ramey wrote:
On 8/17/15 12:19 AM, Vladimir Prus wrote:
On 07-Aug-15 10:08 AM, Bjorn Reese wrote:
Dear Boost and ASIO communities.
The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th.
The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.)
I think that per http://www.boost.org/community/reviews.html the above should have blocked formal review. If a library is reviewed, it normally means the author believes it's to be ready for Boost, which certainly is not the case if one cannot run the test using the current tools.
I have to disagree with this.
a) The above cited link doesn't specify support for boost build as a requirement.
Maybe; though the library as submitted does not follow directory structure guidelines either.
b) No one complained during the review that lack of support for boost build inhibited them from reviewing the submission.
Right; some reviewers also stated they did not try the library at all.
c) The fact that support for boost build may be required for library acceptance doesn't imply that it should be necessary for review.
Certainly, the phrase "The library must come reasonably close to meeting the Guidelines below." is somewhat ambiguous. - Volodya
On 17-Aug-15 6:46 PM, Bjorn Reese wrote:
On 08/17/2015 05:04 PM, Vladimir Prus wrote:
Maybe; though the library as submitted does not follow directory structure guidelines either.
Can you please elaborate on this?
The guidelines say that for libraries with compiled sources, the build description files should go to a 'build' directory. Instead, there are some files at the root level and some under 'cmake' directory. (As an aside, seems like guidelines need to be adjusted for the 'include' directory that is now used everywhere). - Volodya
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Robert Ramey Sent: 17 August 2015 15:38 To: boost@lists.boost.org Subject: Re: [boost] [http] Formal review of Boost.Http
On 8/17/15 12:19 AM, Vladimir Prus wrote:
On 07-Aug-15 10:08 AM, Bjorn Reese wrote:
Dear Boost and ASIO communities.
The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th.
The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.)
I think that per http://www.boost.org/community/reviews.html the above should have blocked formal review. If a library is reviewed, it normally means the author believes it's to be ready for Boost, which certainly is not the case if one cannot run the test using the current tools.
I have to disagree with this.
a) The above cited link doesn't specify support for boost build as a requirement.
b) No one complained during the review that lack of support for boost build inhibited them from reviewing the submission.
c) The fact that support for boost build may be required for library acceptance doesn't imply that it should be necessary for review.
+1 We have often accepted libraries with lots of conditions for changes and improvements. (We very rarely accept libraries with NO conditions for changes and improvements). Without this potential libraries have been and will be lost, wasting all the authors efforts. It is clear from those who understand the topic (including the author), that this library isn't fully baked yet. If discussions can come to agreement on what developments are needed (and that will take some time yet), then accepting subject to mini-re-review may be the best outcome. Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 (0) 1539 561830
On Mon, Aug 17, 2015 at 9:38 AM, Robert Ramey
On 8/17/15 12:19 AM, Vladimir Prus wrote:
On 07-Aug-15 10:08 AM, Bjorn Reese wrote:
Dear Boost and ASIO communities.
The formal review of Vinícius dos Santos Oliveira's Boost.Http library starts today, August 7th and ends on Sunday August 16th.
The source code is build using CMake, but Boost.Build is in the pipeline (already done for documentation.)
I think that per http://www.boost.org/community/reviews.html the above should have blocked formal review. If a library is reviewed, it normally means the author believes it's to be ready for Boost, which certainly is not the case if one cannot run the test using the current tools.
b) No one complained during the review that lack of support for boost build inhibited them from reviewing the submission.
I didn't voice this, but I sat down to go and try out the example. Then I realized that I'd need to get CMake installed and it went into the category of "I'll get around to it later", that "later" never happened. I'm not a particular fan of boost build, but it is what boost is using, so I think it is important for all libraries to support it. Saying 'it will be supported later' isn't something that should be done at the stage of a review. Just my opinion. Tom Kent
participants (17)
-
Agustín K-ballo Bergé
-
Bjorn Reese
-
Darren Cook
-
Glen Fernandes
-
Lee Clagett
-
Niall Douglas
-
Oleg Grunin
-
Paul A. Bristow
-
Ralf Globisch
-
Rob Stewart
-
Robert Ramey
-
Rodrigo Madera
-
Roland Bock
-
Tom Kent
-
TONGARI J
-
Vinícius dos Santos Oliveira
-
Vladimir Prus