[review][beast] Review of Beast starts today : July 1 - July 10

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
110 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
The formal review of Vinnie Falco’s Beast library will take place from
July 1 through July 10, 2017.

Please consider participating in this review. The success of Boost is
partially a result of the quality review process which is conducted by
the community. You are part of the Boost community. I will be grateful
to receive a review based on whatever level of effort or time you can
devote.

Beast is a C++ header-only library serving as a foundation for writing
interoperable networking libraries by providing low-level HTTP/1,
WebSocket, and networking protocol vocabulary types and algorithms using
the consistent asynchronous model of Boost.Asio.

Beast is designed for:

  * Symmetry: Algorithms are role-agnostic; build clients, servers, or
    both.
  * Ease of Use: Boost.Asio users will immediately understand Beast.
  * Flexibility: Users make the important decisions such as buffer or
    thread management.
  * Performance: Build applications handling thousands of connections
    or more.
  * Basis for Further Abstraction: Components are well-suited for
    building upon.

A branch has been made for the review. You can find Beast for review at
these links:

  * Documentation : <http://vinniefalco.github.io/stage/beast/review/>
  * Source code : <https://github.com/vinniefalco/Beast/tree/review>
  * The FAQ answers real questions that come up :

<http://vinniefalco.github.io/stage/beast/review/beast/design_choices/faq.html>

A lightning talk from CppCon 2016 introducing Beast can be found here:
<https://www.youtube.com/watch?v=uJZgRcvPFwI>


Please provide in your review whatever information you think is valuable
to understand your final choice of ACCEPT or REJECT including Beast as a
Boost library. Please be explicit about your decision.

Some other questions you might want to consider answering:

  - What is your evaluation of the design?
  - What is your evaluation of the implementation?
  - What is your evaluation of the documentation?
  - What is your evaluation of the potential usefulness of the library?
  - Did you try to use the library? With which compiler(s)? Did you
    have any problems?
  - How much effort did you put into your evaluation? A glance? A quick
    reading? In-depth study?
  - Are you knowledgeable about the problem domain?

More information about the Boost Formal Review Process can be found
here: <http://www.boost.org/community/reviews.html>

Thank you for your effort in the Boost community.

Happy coding -
michael

--
Michael Caisse
Ciere Consulting
ciere.com

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
> Please consider participating in this review. The success of Boost is
> partially a result of the quality review process which is conducted by
> the community. You are part of the Boost community. I will be grateful
> to receive a review based on whatever level of effort or time you can
> devote.

As this is such a big library, some early first impressions/notes.
Positives:

+ This library is far better quality than when I last looked at it which
was actually probably a long time ago. Congrats to Vinnie on that. It
ain't easy getting it to this far.

+ I pretty much agree with everything in his design rationales in the
section at
https://vinniefalco.github.io/stage/beast/review/beast/design_choices.html
where he compares to other HTTP C++ implementations.

+ I found the docs clear, about the right balance between detail and
simplicity, and I understood everything I felt I needed to know quickly.
Well done on that as well Vinnie, it's a hard balance to achieve.

+ I very much agree with the choices in boundary separating what to
provide and what not to provide, with only a few niggles, but nothing
major and nothing showstopper. I feel a lot of the criticism seen to
date about that choice is meritless once you actually understand real
world implementations of HTTP.


Negatives:

- For this collection of HTTP-related library routines which is what
this library actually is, I do not see why any awareness of ASIO is
needed at all. I don't get why it's needed, and moreover, I think the
library would be far better off removing all awareness of a networking
implementation entirely.

If Beast implemented full HTTP like Boost.Http tried to do, I could see
the value add in tying in tightly to some networking implementation or
other. But it does not implement HTTP, it implements a set of routines
that one would use to implement HTTP instead. Most of the routines Beast
supplies that I examined would be more reusable, easier to use, and less
templatey if they made zero assumptions about what networking
implementation is in use.


- Everything appears to be a template. This sort of API design is forced
on Beast by the tight dependency on ASIO whose public API is almost
entirely templated. I could see that this design choice of ASIO's made
sense in C++ 98 days with the knowledge of best practices then extant in
2005, but in a C++ 11 library designed after 2015 it is the wrong design
choice. We simply know better nowadays, plus WG21 has much superior
vocabulary types for doing buffer sequences than ASIO's which are
needlessly complex, over engineered, and over wraught. A new library
should not repeat the design mistakes of ASIO unless it has to, and I
see no compelling reason given the limited implementation of HTTP
offered by Beast to have any dependency on ASIO at all.

Beast does a great job of drawing the correct boundary around what of
HTTP to implement and what not to implement. I very much agree with that
boundary. But the choice to model ASIO was made superfluous by that
boundary choice. The ASIO design model should be replaced with a more
generic reusable design instead, one which works with any arbitrary
socket API - or indeed, non-socket API.

An an example of a non-socket API, imagine using the UDT stream layer
instead of TCP (http://udt.sourceforge.net/) and implementing HTTP on
top of that. UDT provides its own library implementation and API which
somewhat looks like a socket API, but varies in significant ways.

With Beast's current design, trying to use Beast with the UDT API is
going to be unnatural and awkward. If Beast were instead a reusable,
generic, HTTP related utility library, it would be a much better library
for it.


- The choice to use STL allocators I feel is a code smell. I think a
correct HTTP primitive library would never allocate memory, and thus
never need to call a STL allocator. I think all the places where Beast
does allocate memory were driven by the hard dependency on ASIO, and if
you removed ASIO entirely, you would remove any need for memory allocation.


- I think basic_parser should be designed to use forward iterators
instead of requiring random access, or accept partial input in chunks
whilst keeping state. This would allow it to work seamlessly with
discontiguous underlying storage.


- I appreciate that this my final negative will not be widely agreed
with on boost-dev, but personally speaking I think Beast should work
well with C++ exceptions disabled, and therefore make no use of
exception throws at all. There are lots of end users who would like to
speak HTTP without enabling C++ exceptions for cultural or safety
validation reasons: embedded devices, IoT, games, medical devices etc.
Beast is not useful to any of those use cases if it insists on ASIO and
throws exceptions, neither of which are necessary nor make for a better
HTTP utilities library.



So to sum up, I like the library, most of the design choices are
sensible, I think it would save me a lot of hassle when implementing
HTTP as-is. But I feel it is arse-about-face in design: a simpler, less
cluttered, no-ASIO, no-malloc, less-templates design would be much
better again.

I'm not going to issue a final review now. I specifically want to see
what some others say first, most specifically Bjorn Reese who knows this
area very, very deeply and indeed mentored Boost.Http over multiple
summers. He likely will have a different weighting of priorities to me,
and he's far more a domain expert in this than I am, so his review will
influence mine.

I would also be interested in seeing the proposer's opinion on
everything I've just said before I issue a formal review. I do want to
close with saying that it's a great library Vinnie, you did a great job.
But I think a different design focus would be a lot better again for its
given scope and remit.

Niall

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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
On Sat, Jul 1, 2017 at 7:46 AM, Niall Douglas via Boost
<[hidden email]> wrote:
> + This library is far better quality than when I last looked at it which
> was actually probably a long time ago. Congrats to Vinnie on that. It
> ain't easy getting it to this far.

Thanks! A lot of work went into it in the last couple of months, and
the users chipped in with bug reports and helpful suggestions.

> - For this collection of HTTP-related library routines...

It seems that WebSocket is the red-headed stepchild that no one talks
about. Of course, I understand that the HTTP portions of Beast are far
more interesting (controversial?) so I guess we'll be talking about
that for most of the review.

> ...which is what this library actually is

I think the author of Beast might have some idea of "what this library
actually is", especially when it is stated in the documentation:
<http://vinniefalco.github.io/beast/beast/using_http.html>

> I do not see why any awareness of ASIO is needed at all.

Also stated in the documentation:
"...using the consistent asynchronous model of Boost.Asio."
<http://vinniefalco.github.io/beast/beast/introduction.html>

> I don't get why it's needed,

Boost.Asio is effectively what will become the standard way to do C++
networking (i.e. N4588).  Beast recognizes that C++ will soon have a
common, cross-platform networking interface and builds on that
interface exclusively.

> and moreover, I think the library would be far better
> off removing all awareness of a networking implementation entirely.

Beast stream algorithms are not dependent on any networking
implementation. It only depends on a networking interface.
Specifically, Beast stream algorithms require the following concepts:

    SyncReadStream
    SyncWriteStream
    AsyncReadStream
    AsyncWriteStream
    ConstBufferSequence
    MutableBufferSequence
    DynamicBuffer

It is entirely possible to create your own objects which meet these
requirements that do not use Asio sockets. For example, a libuv socket
can be wrapped in a manner that makes it usable with Beast. None of
the concepts above require a particular networking implementation,
subject to the following caveat:

The Boost.Asio requirements for the stream and buffer concepts listed
above DO require Asio. Specifically they require the following
concrete types (plus a few more related to asynchronous initiating
function customization points which I won't bore readers with):

    boost::asio::io_service
    boost::asio::const_buffer
    boost::asio::mutable_buffer

At the moment, Beast is tied to Boost.Asio because of the
aforementioned types. However, N4588 introduces significant
improvements to the networking interface. The concrete io_service
class is replaced with a new concept called an Executor. The buffer
requirements are relaxed, instead of requiring a concrete type such as
const_buffer it is possible to use any type that offers a data() and
size() member. Thus, N4588 removes the last vestiges of implementation
dependence on code written for it.

My plan for Beast is to port it to N4588 as soon as it becomes
available in an official capacity in the standard libraries used by
clang, gcc, or msvc. So to answer your question, Boost.Asio is needed
today because N4588 is not part of the standard. When it becomes
standardized, Beast will be dependent on the C++ Standard Library
networking interface (interface, not implementation as you stated).

> Most of the routines Beast supplies that I examined would be more reusable,
> easier to use, and less templatey if they made zero assumptions about what
> networking implementation is in use.

beast::http::serializer and beast::http::basic_parser make no
assumptions about what networking implementation is in use. Perhaps
you can demonstrate a sample program using those Beast classes which
makes zero assumptions about which networking implementation is in
use, that works with at least two different implementations; say,
Boost.Asio and libUV?

Personally, I don't see a point to investing resources into supporting
networking implementations which will not become part of the C++
Standard Library.

> WG21 has much superior vocabulary types for doing buffer sequences than ASIO's
> which are needlessly complex, over engineered, and over wraught.

I intend to make sure Beast is compatible with the standard so
whatever buffer sequences make it in to the standard, is what Beast
will use. If you believe that the buffer sequence concepts in N4588
are deficient, I strongly advise you to open a LEWG issue before the
committee makes a terrible mistake! There's still time.

> A new library should not repeat the design mistakes of ASIO unless it has to,
> and I see no compelling reason given the limited implementation of HTTP
> offered by Beast to have any dependency on ASIO at all.

It is only Beast's stream algorithms which use Boost.Asio's stream concepts.

> The ASIO design model should be replaced with a more
> generic reusable design instead, one which works with any arbitrary
> socket API - or indeed, non-socket API.

You can always write your own stream algorithm using
beast::http::basic_parser and beast::http::serializer which do not
work with streams at all and do not require Boost.Asio, subject to the
following caveat:

These Beast classes use Boost.Asio's buffer concepts which are tied to
a couple of concrete types. This dependence will disappear in the port
to N4588, which does not mandate concrete types.

> With Beast's current design, trying to use Beast with the UDT API is
> going to be unnatural and awkward. If Beast were instead a reusable,
> generic, HTTP related utility library, it would be a much better library
> for it.

I'm not sure I agree, you can always implement your own stream
algorithm which uses UDT API and use beast::http::basic_parser and
beast::http::serializer with it. The documentation even provides
examples of how you can use the serializer and parser with
std::ostream and std::istream, bypassing Asio streams entirely:

http://vinniefalco.github.io/beast/beast/using_http/buffer_oriented_serializing.html#beast.using_http.buffer_oriented_serializing.write_to_std_ostream
http://vinniefalco.github.io/beast/beast/using_http/buffer_oriented_parsing.html#beast.using_http.buffer_oriented_parsing.read_from_std_istream

> The choice to use STL allocators I feel is a code smell.

Beast intends to use standard concepts. Allocator and
AllocatorAwareContainer are concepts defined by the C++ standard, and
the universally understood models for allocator customization points.
If you feel that "STL allocators" are a "code smell" then I urge you
to submit a paper to WG21 with actionable advice (ideally, proposed
wording) on how to banish the stench.

> I think a correct HTTP primitive library would never allocate memory,
> and thus never need to call a STL allocator.

beast::http::serializer never allocates memory.
beast::http::basic_parser never allocates memory when its input is a
single buffer. An optimized allocator is provided in the
http-server-fast example which makes basic_fields never allocate from
the heap:
https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/example/http-server-fast/fields_alloc.hpp#L86

Beast also comes with an extensive set of tools to empower users to
avoid memory allocations:

http://vinniefalco.github.io/beast/beast/ref/beast__http__basic_dynamic_body.html
http://vinniefalco.github.io/beast/beast/ref/beast__http__string_view_body.html
http://vinniefalco.github.io/beast/beast/ref/beast__static_buffer.html
http://vinniefalco.github.io/beast/beast/ref/beast__static_buffer_n.html
http://vinniefalco.github.io/beast/beast/ref/beast__static_string.html

Here is a declaration for a message which performs no dynamic
allocations, which allows a a header of up to 4096 bytes and a body of
up to 16384 bytes:

    #include <beast/core.hpp>
    #include <beast/http.hpp>
    #include <example/http-server-fast/fields_alloc.hpp>

    fields_alloc fa{4096};

    beast::http::response<
        beast::http::basic_dynamic_body<beast::static_buffer_n<16384>,
        beast::http::basic_fields<fields_alloc>> res{
            std::piecewise_construct, std::make_tuple(), std::make_tuple(fa)};

> - I appreciate that this my final negative will not be widely agreed
> with on boost-dev, but personally speaking I think Beast should work
> well with C++ exceptions disabled, and therefore make no use of
> exception throws at all.

The DynamicBuffer concept prescribes throwing std::length_error if the
buffer maximum size is exceeded:
<http://vinniefalco.github.io/beast/beast/concept/DynamicBuffer.html>

This is a N4588 concept and will eventually become part of the C++
standard. If you feel that N4588 is deficient because it has a concept
which mandates exceptions, I urge you to open a LEWG issue before the
committee makes a terrible mistake! There's still time.

> There are lots of end users who would like to
> speak HTTP without enabling C++ exceptions for cultural or safety
> validation reasons: embedded devices, IoT, games, medical devices etc.
> Beast is not useful to any of those use cases if it insists on ASIO and
> throws exceptions, neither of which are necessary nor make for a better
> HTTP utilities library.

beast::http::serializer and beast::http::basic_parser do not throw exceptions.

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
On 01/07/2017 17:02, Vinnie Falco via Boost wrote:
>> - For this collection of HTTP-related library routines...
>
> It seems that WebSocket is the red-headed stepchild that no one talks
> about. Of course, I understand that the HTTP portions of Beast are far
> more interesting (controversial?) so I guess we'll be talking about
> that for most of the review.

Beast WebSocket belongs in another, separate library. That library could
bring in ASIO as a dependency.

>> I do not see why any awareness of ASIO is needed at all.
>
> Also stated in the documentation:
> "...using the consistent asynchronous model of Boost.Asio."
> <http://vinniefalco.github.io/beast/beast/introduction.html>
>
>> I don't get why it's needed,
>
> Boost.Asio is effectively what will become the standard way to do C++
> networking (i.e. N4588).  Beast recognizes that C++ will soon have a
> common, cross-platform networking interface and builds on that
> interface exclusively.

I snipped all the hand waving as irrelevant to the question I posed,
which it is.

You didn't answer the question. These are the stated things Beast provides:

1. Message containers

2. Stream reading

3. Stream writing

4. Serialisation

5. Parsing

... which seem a reasonable and useful subset of HTTP to implement.

You have told me no reason why any of these needs a hard dependency on
any networking implementation, or awareness of any specific networking
design pattern.

I personally can see no good reason why they should be: **HTTP has
nothing to do with networking**. It therefore seems to me that reading
ought to be calculated views of underlying byte data, and writing ought
to be compositing a sequence of byte spans to gather into a send. Like
the Ranges TS does.

Please convince me that I am wrong.

Niall

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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Niall Douglas wrote:

> - I appreciate that this my final negative will not be widely agreed with
> on boost-dev, but personally speaking I think Beast should work well with
> C++ exceptions disabled, ...

BOOST_THROW_EXCEPTION abides by best Rust/SG14 practices of aborting on
exception throw when exceptions are disabled. :-)


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 1, 2017 at 7:46 AM, Niall Douglas via Boost <
[hidden email]> wrote:

> There are lots of end users who would like to
> speak HTTP without enabling C++ exceptions for cultural or safety
> validation reasons: embedded devices, IoT, games, medical devices etc.
>

You forgot to mention "unpredictability" of exceptions. :)

Also, there is no error handling in games.


> Beast is not useful to any of those use cases if it insists on ASIO and
> throws exceptions, neither of which are necessary nor make for a better
> HTTP utilities library.
>

They're certainly not necessary for users who don't need robust error
handling.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 1, 2017 at 11:08 AM, Niall Douglas via Boost
<[hidden email]> wrote:
> Beast WebSocket belongs in another, separate library. That library could
> bring in ASIO as a dependency.

Beast is provided as a coherent package of components that work well
together, with integrated tests and documentation. It is presented as
a single library.

> 1. Message containers
> 2. Stream reading
> 3. Stream writing
> 4. Serialisation
> 5. Parsing
> You have told me no reason why any of these needs a hard dependency on
> any networking implementation, or awareness of any specific networking
> design pattern.

(repeating myself)

Beast's message containers, serialization, and parsing do not depend
on any specific networking interface.

I do not know of a way to write an algorithm which works with an
unspecified stream concept, so I had to choose which concepts I wanted
to work with. I chose these Boost.Asio concepts because they are the
closest thing to becoming a standard:

    SyncReadStream
    SyncWriteStream
    AsyncReadStream
    AsyncWriteStream
    DynamicBuffer

Perhaps you can demonstrate how a network algorithm may be written
which works with an unspecified stream concept? How about a simple,
synchronous function that writes a string, I'll start you off with a
function signature:

    template<class Stream>
    void write(Stream& stream, std::string_view string);

Please implement this function for us and demonstrate how it works
with Boost.Asio, Networking-TS, POSIX sockets, WinSock, UDT, or libUV
without modification.

> **HTTP has nothing to do with networking**.

Beast offers algorithms to serialize and parse HTTP messages on
Boost.Asio streams. If you think that is not part of "HTTP" that's
fine, the label is unimportant. What is important is that Beast offers
this functionality.

> It therefore seems to me that reading ought to be calculated views of
> underlying byte data, and writing ought to be compositing a sequence of
> byte spans to gather into a send.

Correct. You have described the operations performed by
beast::http::serializer and beast::http::basic_parser, which do not
require any networking interface.

> WG21 has much superior vocabulary types for doing buffer sequences
> than ASIO's which are needlessly complex, over engineered, and over wraught

Please specify the WG21 vocabulary types you are referring to.

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
>     There are lots of end users who would like to
>     speak HTTP without enabling C++ exceptions for cultural or safety
>     validation reasons: embedded devices, IoT, games, medical devices etc.
>
> You forgot to mention "unpredictability" of exceptions. :)
>
> Also, there is no error handling in games.

Anyone not error handling in HTTP needs their head examined.

Niall

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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
On Sat, Jul 1, 2017 at 12:52 PM, Niall Douglas via Boost
<[hidden email]> wrote:
> Anyone not error handling in HTTP needs their head examined.

I'll kindly ask that you do not make inflammatory statements in
threads which mention Beast in the subject line. Some folks have
relatives or friends who may be mentally ill, and your ad-hominem
implies that those individuals are somehow less worthy of respect.
Such language is non-inclusive and diminishes the stature of the list
(in my opinion).

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 01/07/2017 20:01, Vinnie Falco via Boost wrote:
> On Sat, Jul 1, 2017 at 11:08 AM, Niall Douglas via Boost
> <[hidden email]> wrote:
>> Beast WebSocket belongs in another, separate library. That library could
>> bring in ASIO as a dependency.
>
> Beast is provided as a coherent package of components that work well
> together, with integrated tests and documentation. It is presented as
> a single library.

Except, it is not coherent. You're bundling socket abstraction in with
HTTP parsing and inflicting huge, header-only dependencies on all end
users irrespective of what parts they actually use or need.

At the very minimum, I think Beast needs to become two, separate
libraries: (i) the HTTP utility library (ii) WebSocket.

>> 1. Message containers
>> 2. Stream reading
>> 3. Stream writing
>> 4. Serialisation
>> 5. Parsing
>> You have told me no reason why any of these needs a hard dependency on
>> any networking implementation, or awareness of any specific networking
>> design pattern.
>
> (repeating myself)
>
> Beast's message containers, serialization, and parsing do not depend
> on any specific networking interface.

So why are you forcing end users to drag in ASIO?

The reason why according to you is for the buffer infrastructure. But as
I've already told you, that's a relic from a decade ago. New code
neither ought to nor needs to use that. We have far better available today.

And if the really reusable parts of Beast, the ones not dependent on
ASIO except for some buffer adapters, can be broken off and be made free
of ASIO, that's a big value add to end users who don't need WebSocket
and just want a HTTP utilities library.

> I do not know of a way to write an algorithm which works with an
> unspecified stream concept, so I had to choose which concepts I wanted
> to work with. I chose these Boost.Asio concepts because they are the
> closest thing to becoming a standard:
>
>     SyncReadStream
>     SyncWriteStream
>     AsyncReadStream
>     AsyncWriteStream
>     DynamicBuffer
>
> Perhaps you can demonstrate how a network algorithm may be written
> which works with an unspecified stream concept? How about a simple,
> synchronous function that writes a string, I'll start you off with a
> function signature:
>
>     template<class Stream>
>     void write(Stream& stream, std::string_view string);

You're thinking in terms of i/o. Stop doing that. Very little in your
library has, or ought to have, anything to do with i/o. It's the major
flaw in your library's design as I see it.

Think in terms of blobs of bytes. Blobs of bytes come in. Blobs of bytes
go out. No i/o. No reading, no writing. Just blobs of bytes.

>> **HTTP has nothing to do with networking**.
>
> Beast offers algorithms to serialize and parse HTTP messages on
> Boost.Asio streams. If you think that is not part of "HTTP" that's
> fine, the label is unimportant. What is important is that Beast offers
> this functionality.

i/o, ASIO, networking and all this stuff has confounded the clarity and
intent of your design which is very solid.

Stop thinking in terms of i/o. HTTP is just structured data. So treat it
as such. Parse as structured data, generate as structured data.

The natural split point for Beast into multiple, focused libraries is
between the code which only concerns itself with structured data, and
everything else.

>> WG21 has much superior vocabulary types for doing buffer sequences
>> than ASIO's which are needlessly complex, over engineered, and over wraught
>
> Please specify the WG21 vocabulary types you are referring to.

The Ranges TS is an obvious place to start from as a source of Concepts
and vocabulary types to draw from. You don't actually need a Ranges TS
as all your Views, InputRanges, OutputRanges etc. will be of char or
const char anyway. These are very easy to knock together, indeed my GSoC
student this year Tom knocked together a constexpr implementation of
those in less than a week. When the Ranges TS is available in your
compiler, of course patch in that if the end user configures a macro for
it, but you can write just enough of a Ranges TS implementation to suit
all your needs very quickly.

If that's too experimental for you (though the Ranges TS is a TS just
like the Networking TS), then do as Boost.Spirit does and work with
iterator pairs. So for example, if I want to know what the
Content-Length header is, upon query I get back an iterator pair
pointing to the front of the storage and one after the end of the
storage. Similarly, if I set the Content-Length header, I supply an
iterator pair to the new contents.

Niall

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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
On Sat, Jul 1, 2017 at 1:38 PM, Niall Douglas via Boost
<[hidden email]> wrote:
> At the very minimum, I think Beast needs to become two, separate
> libraries: (i) the HTTP utility library (ii) WebSocket.

Beast is proposed as-is.

> The natural split point for Beast into multiple, focused libraries is
> between the code which only concerns itself with structured data, and
> everything else.

Beast is proposed as-is.

> So why are you forcing end users to drag in ASIO?

Beast will be part of Boost, whose distributions include Asio. To use
serializer or basic_parser,  the header <boost/asio/buffer.hpp> must
be included, which is a pretty self-contained file. This is a
temporary situation until Boost.Asio is updated to N4588.

> The reason why according to you is for the buffer infrastructure. But as
> I've already told you, that's a relic from a decade ago. New code
> neither ought to nor needs to use that. We have far better available today.

That "relic from a decade ago" is in the latest Boost 1.65.0. As I
have targeted Boost.Asio specifically, you will naturally understand
that Beast uses that buffer infrastructure for better or for worse.
When a version of Boost.Asio appears which is "far better", then Beast
will be ported to it.

The stand-alone Asio is already a bit ahead of Boost.Asio in its
buffer technologies; since you feel that Boost.Asio is so far behind
perhaps you can take on the task of porting stand-alone Asio's changes
to Boost?

> And if the really reusable parts of Beast, the ones not dependent on
> ASIO except for some buffer adapters, can be broken off and be made free
> of ASIO, that's a big value add to end users who don't need WebSocket
> and just want a HTTP utilities library.

Eliminating the dependence on Asio's buffers is something that is on
my horizon, but it is not a feature for the library being proposed.
There hasn't been much demand for using the parser or serializer
without <boost/asio/buffer.hpp>. But if that were to change, I would
likely reprioritize the feature. A port to N4588 will automatically
solve the problem.

>>> WG21 has much superior vocabulary types for doing buffer sequences
>>> than ASIO's which are needlessly complex, over engineered, and over wraught
>>
>> Please specify the WG21 vocabulary types you are referring to.
>The Ranges TS is an obvious place to start from as a source of Concepts

Until you file the LEWG issue where you describe N4588 buffer sequence
concepts as "needlessly complex, over engineered, and over
wraught[sic]" and provide an alternative in the form of proposed
language, there is nothing actionable in your statement.

> Very little in your library has, or ought to have, anything to do with i/o.
> ..
> Stop thinking in terms of i/o. HTTP is just structured data. So treat it
> as such. Parse as structured data, generate as structured data.

Is my understanding correct that you say Beast should not provide
functions to send and receiving HTTP messages using Asio streams?

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 1, 2017 at 1:38 PM, Niall Douglas via Boost <
[hidden email]> wrote:

> The reason why according to you is for the buffer infrastructure. But as
> I've already told you, that's a relic from a decade ago. New code
> neither ought to nor needs to use that. We have far better available today.
>

"A relic from a decade ago" is not an argument.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 1, 2017 at 12:52 PM, Niall Douglas via Boost <
[hidden email]> wrote:

> >     There are lots of end users who would like to
> >     speak HTTP without enabling C++ exceptions for cultural or safety
> >     validation reasons: embedded devices, IoT, games, medical devices
> etc.
> >
> > You forgot to mention "unpredictability" of exceptions. :)
> >
> > Also, there is no error handling in games.
>
> Anyone not error handling in HTTP needs their head examined.
>

You mean error checking, I mean handling and recovery. Games tend to just
retry and eventually reboot on errors, rather than tear-down data
structures and recover gracefully. It's one reason why game programmers
don't see value in exception handling.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 1, 2017 at 2:39 AM, Michael Caisse via Boost <
[hidden email]> wrote:

> The formal review of Vinnie Falco’s Beast library will take place from
> July 1 through July 10, 2017.
>
> Please consider participating in this review. The success of Boost is
> partially a result of the quality review process which is conducted by
> the community. You are part of the Boost community. I will be grateful
> to receive a review based on whatever level of effort or time you can
> devote.
>

Aright, you got me.

[snip]

One thing up top: igzf what you call your library.  You're the author, you
get to name it.  We have Spirit (w/Qi and Karma), Hana, Phoenix, and Proto
already.  No one can claim that convention around here is that a library's
name matches its uses.

Some other questions you might want to consider answering:
>
>   - What is your evaluation of the design?
>

* Overall, I quite like it!  I have some complaints, but they are about
small design choices.  The largest design choices, of adhering closely to
the now-begin-standardized ASIO/Net-TS, and to stay in a layer just above
ASIO (and not much higher), are the right ones.  Also, making the library
explicitly standards-tracked is a good choice.  We need more of that in
Boost these days.

* Allocators are gross.  I know this is a controversial position, but I
think they're far more trouble than they're worth.  Since you already have
buffers that seem to me to cover all the major allocation strategies
already, can't you get away with adding a buffer adapter that takes any
random access container as its underlying storage, and be done with it?
This is an effort to reduce your maintenance workload, and the semantic
overhead of users.  Introducing allocators and then trying to use them
uniformly has lead to some awful stuff in the standard, like allocators in
the std::pair and std::tuple ctors, even though they do not allocate.  Yuck.

* Is there a reason why buffer_cat_view and buffer_prefix_view are distinct
types?  It isn't obvious just from their descriptions.  Moreover, fewer
user-facing types is better.  I would greatly prefer to write code like
this:

buffers_view<BufT0, BufT1, ...> buffers = buffer_cat(buf0, buf1, ...);
buffers_view<BufT0, BufT1, ...> buffers_slice = buffer_slice(0, prefix_end);
buffers = buffers_slice(offset, size);

To accomplish this, I think you just need to make a single type that owns a
sequence of buffers, and maintains a pair of iterators or indices into the
owned sequence.  If there is a deeper reason that I have not noticed for
why the current design should remain as-is, please add this to a Rationale
section.

* Similarly, as a user I'd prefer not to have string_body as a distinct
type (unless it's a convenience typedef for dynamic_body<std::string>).  If
std::string is not a model of DynamicBuffer, I think DynamicBuffer could
stand a slight reformulation.

* I get why message<> has a Body template parameter, but why is Fields
configurable?  There is no message<> instantiation under examples/ or
include/ that uses anything other than basic_fields<> that I could find.
Do people ever use something else?  Again, a brief rationale would be
useful.  I saw the "HTTP Message Container" page, btw, but while it
suggested someone might want a different Fields template param, it didn't
indicate why.

  - What is your evaluation of the implementation?
>

I did not look much at the implementation, except as quick checks against
the documentation.


>   - What is your evaluation of the documentation?
>

Very good.  It has lot and lots of concrete examples of how to use
different parts of the library in many different ways.  That's really
important for a large library.

* The Quick Start contains its subsections in full, and then there are TOC
links to each subsection separately.  Probably one or the other would do.

* Each of the example links takes me to a GitHub page for the file linked.
That should change to inline source like the Quick Start code.  The same
thing is true of all the reference documentation.

* HTTP Crawl seems to be doing synchronous reads.  Is there an example
elsewhere that does a bunch of reads like this asynchronously?

* example/http-server-fast/http_server_fast.cpp explicitly checks for '/'
path separators, and uses strings instead of filesystem::paths.  It would
be cool if the former were changed for Windows folks, and the latter
changed, just 'cuz.

* At least http_server_small.cpp (and maybe others? I didn't go back and
check) has only Christopher M. Kohlhoff as an author.  Vinnie, you probably
need a copyright audit of all your files.

* I found the mix of function and type entries in "Table 6. Buffer
Algorithms" to be initially quite confusing.  Maybe separate them?

* The buffer_prefix_view documentation in "Table 6. Buffer Algorithms"
 says "This is the type of buffer returned by buffer_prefix.", but 2/3
overloads with that name return something else.

* HTTP Message Container says this:

"
Now we can declare a function which takes any message as a parameter:

template<bool isRequest, class Fields, class Body>
void f(message<isRequest, Fields, Body>& msg);
This function can manipulate the fields common to requests and responses.
If it needs to access the other fields, it can do so using tag dispatch
with an object of type std::integral_constant<bool, isRequest>.
"

This strikes me as odd advice.  With pre-C++17 I would write:

template<class Fields, class Body>
void f(message<true, Fields, Body>& msg)
{ /*...*/ }

template<class Fields, class Body>
void f(message<false, Fields, Body>& msg)
{ /*...*/ }

Why would I have a third overload and use tag dispatching?  Most users are
going to be really confused by what I quoted above, I think.

Of course, with C++17 and later I'd write:

template<bool isRequest, class Fields, class Body>
void f(message<isRequest, Fields, Body>& msg)
{
    if constexpr (isRequest) {
        // ...
    } else {
        // ...
    }
}

.. and that lines up nicely with the given rationale (just not the tag
dispatching bit).

* Please add an explicit Rationale section for smaller design choices.  The
FAQ and design justification sections that exist are fine, but they mostly
cover the biggest-picture items. Each of those small design choices are
going to go out the window if you can't remember a strong argument for one
of them 5 years from now, when LEWG are asking you why a certain interface
is the way it is.

  - What is your evaluation of the potential usefulness of the library?
>

High.  Super duper high.  The fact that I have no standard way to do
anything in this library today, and this library is
standardization-oriented is a great thing.


>   - Did you try to use the library? With which compiler(s)? Did you
>     have any problems?
>

I did not use the library.


>   - How much effort did you put into your evaluation? A glance? A quick
>     reading? In-depth study?
>

I spent about 3 hours reading documentation and examples, with a quick
glance at some of the implementation.


>   - Are you knowledgeable about the problem domain?


Somewhat.  I've written a couple of Node.js-based web servers, a couple of
straight-ASIO non-web servers and clients, and done a bit of web
development.  I've done nothing that works at exactly the level Beast
targets.

Zach

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
On Sat, Jul 1, 2017 at 4:42 PM, Zach Laine via Boost
<[hidden email]> wrote:

Thanks for checking out the library!

> * Is there a reason why buffer_cat_view and buffer_prefix_view are distinct
> types?

Yes, buffer_cat_view and buffer_prefix_view are two completely
different things. A buffer_cat_view represents a buffer sequence of
buffer sequences, while a buffer_prefix_view is just a range adapter
over a single buffer sequence.

> I think you just need to make a single type that owns a sequence of buffers,
> and maintains a pair of iterators or indices into the owned sequence.

You've more or less described buffer_prefix_view.

A rough description of buffer_cat_view would be "a type that owns a
heterogenous list of buffer sequences, whose iterators reflect the
current position within a variant consisting of each iterator types
from the list of buffer sequences." Quite different :)

> * Similarly, as a user I'd prefer not to have string_body as a distinct
> type (unless it's a convenience typedef for dynamic_body<std::string>).  If
> std::string is not a model of DynamicBuffer, I think DynamicBuffer could
> stand a slight reformulation.

std::string does not model DynamicBuffer, which itself is a Net-TS
concept and thus immutable from Beast's perspective. Therefore
beast::http::basic_dynamic_body cannot work with std::string and
beast::http::string_body becomes necessary . I will likely add
beast::http::basic_string_body to allow the allocator to be
customized.

> * I get why message<> has a Body template parameter, but why is Fields
> configurable?  There is no message<> instantiation under examples/ or
> include/ that uses anything other than basic_fields<> that I could find.
> Do people ever use something else?  Again, a brief rationale would be
> useful.  I saw the "HTTP Message Container" page, btw, but while it
> suggested someone might want a different Fields template param, it didn't
> indicate why.

I have not done extensive work in this area of the library yet but I
assure you it is a useful feature. A custom Fields implementation
could support only a subset of all possible fields (for example it
might only be capable of representing Content-Length,
Transfer-Encodoing, Server, and User-Agent), storing the field name in
a small integer enum instead of keeping the entire text. It could
store Content-Length as an integer instead of a string. Custom Fields
could use its own strategy for storing the field/value pairs.

Most importantly, a custom Fields could be used to adapt another
library's message model into something that Beast can consume. Or to
allow a Beast message to be consumable by another library with a
proper wrapper. All of these contemplated use-cases require the user
to also provide a subclass of basic_parser, as the beast::http::parser
is only usable with beast::http::basic_fields.

These use-cases seem exotic but I feel that the niches they satisfy
will come up and have merit. Think embedded devices or Internet of
Things. Creating examples of custom Fields is on my to-do:
<https://github.com/vinniefalco/Beast/issues/504>

> * Allocators are gross.  I know this is a controversial position,

I agree, that is controversial! And I'm not entirely happy with the
interface of AllocatorAwareContainer but it is what it is. Which is to
say it is standardized.

> I think they're far more trouble than they're worth.  Since you already have
> buffers that seem to me to cover all the major allocation strategies
> already, can't you get away with adding a buffer adapter that takes any
> random access container as its underlying storage, and be done with it?

That already exists, you can use beast::buffers_adapter on any linear
buffer (including one provided by a container) and then use that
adapter with beast::http::basic_dynamic_body.

There are only a few AllocatorAwareContainer in Beast, those include
basic_flat_buffer, basic_multi_buffer, but most importantly
http::basic_fields. I don't think removing allocator support in
http::basic_fields is such a good idea, especially when one of the
examples already demonstrates how performance may be improved through
its use:
<https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/example/http-server-fast/fields_alloc.hpp#L86>

Folks who want to get the most out of Beast will be taking advantage
of Beast's allocator support so I must respectfully disagree with your
position.

> * The Quick Start contains its subsections in full, and then there are TOC
> links to each subsection separately.  Probably one or the other would do.

This is intended, I want every example to have a ToC entry so that
users can go to it quickly. Maybe if they forget the section it was in
then the Toc entry will jog their memory.

> * Each of the example links takes me to a GitHub page for the file linked.
> That should change to inline source like the Quick Start code.

Hmm I don't know about that, the server-framework is enormous.
Including the example source code in bulk would make it hard to
navigate. And I plan on more examples, I don't think that scales. Of
course, if the consensus is that this change reflects an improvement
then I will make it gladly. I have opened this issue for discussion:
<https://github.com/vinniefalco/Beast/issues/565>

>The same thing is true of all the reference documentation.

I'm not sure what you mean here.

> * HTTP Crawl seems to be doing synchronous reads.  Is there an example
> elsewhere that does a bunch of reads like this asynchronously?

Not yet but I can add something like that:
<https://github.com/vinniefalco/Beast/issues/566>

> * At least http_server_small.cpp (and maybe others? I didn't go back and
> check) has only Christopher M. Kohlhoff as an author.  Vinnie, you probably
> need a copyright audit of all your files.

The copyright is correct, Chris is the author of http_server_small.cpp
and http_server_fast.cpp

> * I found the mix of function and type entries in "Table 6. Buffer
> Algorithms" to be initially quite confusing.  Maybe separate them?

How about changing the title to "Buffer Algorithms and Types"?

> * The buffer_prefix_view documentation in "Table 6. Buffer Algorithms"
>  says "This is the type of buffer returned by buffer_prefix.", but 2/3
> overloads with that name return something else.

Yeah that's a little confusing. I'll sort it:
<https://github.com/vinniefalco/Beast/issues/567>

> * HTTP Message Container says this:
> ...
> If it needs to access the other fields, it can do so using tag dispatch
> ...
> Why would I have a third overload and use tag dispatching?  Most users are
> going to be really confused by what I quoted above, I think.

You're right. I think I had functions like this in mind:
<https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/include/beast/http/impl/basic_parser.ipp#L347>

The doc is a bit confusing, I will fix it.

> * Please add an explicit Rationale section for smaller design choices.  The
> FAQ and design justification sections that exist are fine, but they mostly
> cover the biggest-picture items. Each of those small design choices are
> going to go out the window if you can't remember a strong argument for one
> of them 5 years from now, when LEWG are asking you why a certain interface
> is the way it is.

Good advice but either vague or broad in scope (every design choice?),
if there are specific design choices that should be explained then
this becomes more actionable:
<https://github.com/vinniefalco/Beast/issues/569>

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 1, 2017 at 6:42 PM, Zach Laine <[hidden email]>
wrote:

> On Sat, Jul 1, 2017 at 2:39 AM, Michael Caisse via Boost <
> [hidden email]> wrote:
>
>> The formal review of Vinnie Falco’s Beast library will take place from
>> July 1 through July 10, 2017.
>>
>> Please consider participating in this review. The success of Boost is
>> partially a result of the quality review process which is conducted by
>> the community. You are part of the Boost community. I will be grateful
>> to receive a review based on whatever level of effort or time you can
>> devote.
>>
>
[snip]

And I vote to accept Beast.  Sigh.  It's always something.

Zach

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Sat, Jul 1, 2017 at 7:22 PM, Vinnie Falco via Boost <
[hidden email]> wrote:

> On Sat, Jul 1, 2017 at 4:42 PM, Zach Laine via Boost
> <[hidden email]> wrote:
>
> Thanks for checking out the library!
>
> > * Is there a reason why buffer_cat_view and buffer_prefix_view are
> distinct
> > types?
>
> Yes, buffer_cat_view and buffer_prefix_view are two completely
> different things. A buffer_cat_view represents a buffer sequence of
> buffer sequences, while a buffer_prefix_view is just a range adapter
> over a single buffer sequence.
>
> > I think you just need to make a single type that owns a sequence of
> buffers,
> > and maintains a pair of iterators or indices into the owned sequence.
>
> You've more or less described buffer_prefix_view.
>

Right.


> A rough description of buffer_cat_view would be "a type that owns a
> heterogenous list of buffer sequences, whose iterators reflect the
> current position within a variant consisting of each iterator types
> from the list of buffer sequences." Quite different :)


They don't seem that different to me.  The data look the same from the
outside, but the underlying structure is different.  The underlying
structure doesn't seem like it *needs* to be different, though.  One could
be flattened into the other.  These are all views, right?  Cheap-to-copy
views?  So, in terms of the existing types, if buffer_cat() returned a
buffer_prefix_view with n=0, would that break things?  I mean a semantic
breakage, not a compilation breakage.


> > * Similarly, as a user I'd prefer not to have string_body as a distinct
> > type (unless it's a convenience typedef for dynamic_body<std::string>).
> If
> > std::string is not a model of DynamicBuffer, I think DynamicBuffer could
> > stand a slight reformulation.
>
> std::string does not model DynamicBuffer, which itself is a Net-TS
> concept and thus immutable from Beast's perspective. Therefore
> beast::http::basic_dynamic_body cannot work with std::string and
> beast::http::string_body becomes necessary . I will likely add
> beast::http::basic_string_body to allow the allocator to be
> customized.


I get it.  That's a shame.


> > * Allocators are gross.  I know this is a controversial position,
>
> I agree, that is controversial! And I'm not entirely happy with the
> interface of AllocatorAwareContainer but it is what it is. Which is to
> say it is standardized.


It's not that uncommon an opinion these days.  Be mentally prepared for a
hard left turn wrt allocators (which may never come), if this finally makes
it to LEWG.


> > * Please add an explicit Rationale section for smaller design choices.
> The
> > FAQ and design justification sections that exist are fine, but they
> mostly
> > cover the biggest-picture items. Each of those small design choices are
> > going to go out the window if you can't remember a strong argument for
> one
> > of them 5 years from now, when LEWG are asking you why a certain
> interface
> > is the way it is.
>
> Good advice but either vague or broad in scope (every design choice?),
> if there are specific design choices that should be explained then
> this becomes more actionable:
> <https://github.com/vinniefalco/Beast/issues/569>
>

Well, certainly not every choice.  But definitely every nonobvious choice,
or a choice that looks from the outside like taste, should go in a
rationale section. imo.

Zach

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
On Sat, Jul 1, 2017 at 6:20 PM, Zach Laine via Boost
<[hidden email]> wrote:
> They don't seem that different to me.  The data look the same from the
> outside, but the underlying structure is different.  The underlying
> structure doesn't seem like it *needs* to be different, though.  One could
> be flattened into the other.  These are all views, right?  Cheap-to-copy
> views?  So, in terms of the existing types, if buffer_cat() returned a
> buffer_prefix_view with n=0, would that break things?  I mean a semantic
> breakage, not a compilation breakage.

Perhaps it is possible to combine the functionality of
buffer_prefix_view and buffer_cat_view, but it would come at the
expense of a larger object. Buffer sequences are supposed to be
copyable and normally they are not particularly large. For example
beast::multi_buffer::const_buffers_type costs just one pointer and
therefore pretty darn cheap. But some parts of Beast declare truly
gnarly buffer sequences, like this one:
<https://github.com/vinniefalco/Beast/blob/78a065ba39836d91d7e70d93de7f9140f518083b/include/beast/http/serializer.hpp#L190>

    using ch2_t = consuming_buffers<buffer_cat_view<
       detail::chunk_header,
       boost::asio::const_buffers_1,
       boost::asio::const_buffers_1,
       typename reader::const_buffers_type,
       boost::asio::const_buffers_1,
       boost::asio::const_buffers_1,
       boost::asio::const_buffers_1,
       boost::asio::const_buffers_1>>;

reader::const_buffers_type can itself be a buffer_cat_view or some
other sort of adapted buffer sequence, it depends on the traits of the
Body being used.

Chris was very helpful in working with me to optimize the
http-server-fast example, he pointed out that the size of this buffer
was exactly 256 bytes which unfortunately is one byte larger than it
needs to be in order to take advantage of an Boost.Asio implementation
optimization. I got lucky and found an unused member in
consuming_buffers which I was able to remove.

The moral of the story is that sizeof(ConstBufferSequence) counts, so
these buffer range adapters need to be separate in order to be as
small as possible.

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
>> The reason why according to you is for the buffer infrastructure. But as
>> I've already told you, that's a relic from a decade ago. New code
>> neither ought to nor needs to use that. We have far better available today.
>>
>
> "A relic from a decade ago" is not an argument.

It would not be if there hadn't been very significant progress in how to
do generic and powerful buffers and views since.

However, we have the Ranges TS now, we have Contiguous Container concept
support, we have spans and views, all on the standards track. ASIO gets
to keep its legacy design because it's so important to standardise
existing practice, not invent standards by committee. But any new code
must be held to a higher standard.

And most especially in this particular case, dragging in the entire of
ASIO for a few buffer types is just crazy daft. Instead use some Ranges
TS inspired types - or a C++ 98 era iterator pair - from which ASIO can
auto construct buffer sequences on its own. Do not drag in ASIO for such
a trivial, and unnecessary, use case, so gratuitously damaging the end
user experience as a result.

Niall

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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review][beast] Review of Beast starts today : July 1 - July 10

Boost - Dev mailing list
On Sun, Jul 2, 2017 at 8:07 AM, Niall Douglas via Boost
<[hidden email]> wrote:
>...

> Very little in your library has, or ought to have, anything to do with i/o.

Is my understanding correct that you say Beast should not provide
functions to send and receive HTTP messages using Asio streams?

Thanks

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
1234 ... 6
Loading...