Here's my Review: DESIGN ====== The design of concepts, data structures and algorithms is mostly done well, with only minor concerns: - The smell of two-phase initialization. Required message properties like method and target have to be set from outside. This makes me as a library user afraid of forgetting something vital. The same holds for prepare_payload(). If I forget to call prepare_payload() or forget to set one of the required message properties or fields, my program may - in the worst case - appear to work correctly. - Inconsequent application of automagic. WebSocket control frames are automagically responded to. Iff there's an active read(). Basically, having an active read() at almost all times is a requirement of any correct program using Beast. Even though I enjoy the help from the library, and the requirement is usually met, I found this specific behaviour to be inconsequent. Also, the very first WebSocket example in the docs makes it clear that while a close from the peer is handled automagically under the above condition, a close from my side is quite tedious to write. There should be a utility in the library to do the draining for me. - The naming of BodyReader and BodyWriter initially got me confused. In my mind, I 'read' a body FROM a stream/buffer, I 'write' it INTO a stream or buffer. The library uses the terms in the reverse sense. This is not incorrect of course, but my initial confusion may be a sign of suboptimal naming. IMPLEMENTATION ============== Didn't look in-depth. DOCUMENTATION ============= The documentation is very well structured. It is complete in many, but not all places. - A minimalistic HTTP server example would have helped me get started. Just something very, very basic. Reply to each request with "your request was". - In some places, non-self-explanatory function names suddenly pop up without being first introduced. For example, in the section "Using WebSocket"."Send and Receive Messages", text() and got_text() could use some explanation. In the same section, the reference to set_option() seems out-of-place and makes me wonder what I missed. - Since the documentation features comparison with other libraries, it may be worth comparing pion (https://github.com/splunk/pion) as well. - Many reference pages could use some prosaic explanation. For example, my uncertainty concerning message.prepare_payload() might have been alleviated, were it more clear what the function is intended for and what the effect of omitting it would be. SCOPE ===== Since the FAQ is quite defensive in regard to the library's scope, I feel this is worth some discussion. I found the scope to be just right for now, but with future potential. As it is, the library takes away all the tedious protocol stuff, while leaving all important design decisions to the user. It feels like an extension to ASIO. However, there's no apparent reason why (e.g.) request targets shouldn't be URL-decoded, and why form data should not be parsed into a map of some kind. The way I see it, none of this added funtionality would interfere with the library's design. It would, however, make the library user's life a whole lot easier, and probably attract more users as well. USEFULNESS ========== See SCOPE. Very valuable, could be even better. HOW MUCH EFFORT & TRIED TO USE ============================== I invested about a day into the review, read the documentation and ported a small REST-like service from pion to Beast. I may be biased towards Beast due to temporal proximity, but I feel the latter made the code easier to reason about and, at the very least, did not hurt performance. I used gcc 7.1 and clang 4.0. On the review branch, none of the two had any issues. I deliberately did NOT follow the discussion on the mailing list, in order to remain as unbiased as practicable. HOW KNOWLEDGABLE AM I? ====================== I have worked on several HTTP servers and experimented with writing an HTTP server library a few years back. I have no practical experience with WebSockets. IN ONE SENTENCE =============== Beast should be ACCEPTED into Boost. Marcel
On Wed, Jul 5, 2017 at 2:31 AM, Marcel Ebmer via Boost
Here's my Review:
Thanks you!
Required message properties like method and target have to be set from outside. This makes me as a library user afraid of forgetting something vital.
message will have constructor variations for setting the header values https://github.com/vinniefalco/Beast/commit/8d677757645405295a1417b703c19f2d...
The same holds for prepare_payload(). If I forget to call prepare_payload() or forget to set one of the required message properties or fields
WebSocket control frames are automagically responded to. Iff there's an active read(). Basically, having an active read() at almost all times is a requirement of any correct program using Beast. Even though I enjoy the help from the library, and the requirement is usually met, I found this specific behaviour to be inconsequent.
What's the call to action? What change is being proposed?
There should be a utility in the library to do the draining for me.
willfix https://github.com/vinniefalco/Beast/issues/564
However, there's no apparent reason why (e.g.) request targets shouldn't be URL-decoded, and why form data should not be parsed into a map of some kind. The way I see it, none of this added funtionality would interfere with the library's design.
"out of scope": that dreaded phrase that no one wants to hear :) The features you described are arbitrary. Why forms and not basic authentication? Why URL decoding but not a full URI parser? Why a full URI parser but no handling for redirects? What about supporting proxies? Anything that I add is going to satisfy some users and disappoint others. That's why I'd like to keep Beast as it is, a low level protocol layering on top of Asio. I am carefully and strategically adding things to Beast where it makes sense. For example, based on feedback I am making file_body a public interface. Taking advantage of platform specific optimizations to send files as HTTP message payloads is most certainly in scope. Dealing with the actual content of the message, is not - subject to the caveat that Beast must understand enough of the message to reliably get it from one end to the other. That's why I could accept the argument that supporting alternate status-lines (e.g. "ICY 200 OK\r\n") should be in-scope. That doesn't mean that I won't be building new higher level features (I'm already working on a URL decoder) but I think they belong in a separate library with its own documentation, tests, CI infrastructure, and also separate Boost formal review process. Still, there's only so much I can do - HTTP is a MASSIVE domain!. The only reason I was able to get Beast to where it is at was by being strict on what features I would support. I'm hopeful that other people will build on top of Beast and propose their own new libraries to fill in these gaps. Every so often, students or interns come to the mailing list and ask how they can help. Here's an idea, port this library to modern C++ and Beast: http://www.codesink.org/mimetic_mime_library.html Thanks
WebSocket control frames are automagically responded to. Iff there's an active read(). Basically, having an active read() at almost all times is a requirement of any correct program using Beast. Even though I enjoy the help from the library, and the requirement is usually met, I found this specific behaviour to be inconsequent. What's the call to action? What change is being proposed? I don't have a concrete proposal. What I was trying to describe is the discrepancy between the general look and feel of the library - where the user is in full control, and hence bears full responsibility - and the automagic happening with WebSocket control frames. And, to make it feel even more awkward, the automagic is actually semi-automatic. I realize
"out of scope": that dreaded phrase that no one wants to hear :)
The features you described are arbitrary. Why forms and not basic authentication? Why URL decoding but not a full URI parser? Why a full URI parser but no handling for redirects? What about supporting proxies? Anything that I add is going to satisfy some users and disappoint others. I don't agree. Any feature you add will satisfy some users, and not-yet-satisfy others. Big difference. That doesn't mean that I won't be building new higher level features (I'm already working on a URL decoder) but I think they belong in a separate library with its own documentation, tests, CI infrastructure, and also separate Boost formal review process. Still, there's only so much I can do - HTTP is a MASSIVE domain!. The only reason I was able to get Beast to where it is at was by being strict on what features I would support. For a new library, the expectations would be very high. It would have to support all of HTTP - which may well mean that such a swiss army knife
On 07/05/2017 04:13 PM, Vinnie Falco wrote: there's probably no sane way to have control frames always responded to during the lifetime of a websocket::stream. It may, however, be reasonable to give control back to the user and just provide convenient utilities for sending the appropriate responses. library will never happen. For an existing library, like Beast, the level of expectation (concerning new features) is a lot more bearable. Anything you give us will be seen as a goodie. Beast CAN add an URI parser without supporting Basic Authentification. Or Basic Auth without supporting form data at the same time. You could also put the functionality into a utility namespace and exclude that from your standards proposal.
participants (2)
-
Marcel Ebmer -
Vinnie Falco