Receive/parse the message body one chunk at a time

[georgi-d]

I really like the library but am still missing one critical functionality, to be able to read the message one chunk (HTTP chunk on the wire) at a time. I would want to have a parser for which async_parse can be called multiple times to fill the message one chunk at a time. After each completion of async_parse I should be able to either drain the message body to reduce memory usage or leave it there so the next async_parse would append to the body buffer. I think this functionality is available for websockets.

Another useful feature would be to be able to specify async_parse to complete after reading at most X amount of bytes in the body. Calling async_parse again should continue appending data to the message body.

[vinniefalco]

Incremental processing for the body is already possible, but with a different interface than what you are asking for. If you want to process the body incrementally then you simply need to create your own Body type, and then customize the Reader:

Requirements for Body http://vinniefalco.github.io/beast/beast/ref/Body.html

Requirements for Reader http://vinniefalco.github.io/beast/beast/ref/Reader.html

Example user-defined body:

// incrementally reads body data
struct MyBody
{
  using value_type = std::string; // can be any type you want

  struct reader
  {
    template<bool isRequest, class Headers>
    reader(message<isRequest, MyBody, Headers>& m);

    void
    write(void const* data, std::size_t size, error_code& ec)
    {
      // this will be called with each piece of the body, after chunk decoding
  };
};

Another useful feature would be to be able to specify async_parse to complete after reading at most X amount of bytes in the body

Why do you want to do this? What is the use-case?

[georgi-d]

The Reader customization is a nice feature but as well as any other callback mechanisms it does not provide the functionality to suspend/throttle the receive until another async operation completes. The only option is to store the whole message body and then start a different async operation.

Imagine a reverse proxy implementation that receives an HTTP message from a client parses the headers and then based on the path creates a new http request to a backend server then passes all the data received from the client to the server.

The workflow would be like this:

{  
  parse_headers(sock, hp, msg);
  // check the path
  connect(backend, beSock);
  error_code ec;
  request beReq;

  for (parse_atmost(sock, bp, msg, 1024); 
         ec == error_more_data;
        parse_atmost(sock, bp, msg, 1024))
  {
      write_chunk(beSock, beReq, msg.data());
  }
}

I think an error code can be used to indicate that the parsing was stopped before reaching the end of the message.

To make things symmetrical a similar mechanism would be needed on the write side as well. To send the data chunk by chunk.

[vinniefalco]

I agree, Reader does not provide the ability to throttle. However, I think the responsibility of throttling lies not with the HTTP implementation but rather, the choice of the AsyncReadStream parameter used to parse: http://vinniefalco.github.io/beast/beast/ref/http__async_parse.html

If you want to throttle, then you should write a class that meets the requirements of AsyncReadStream (http://www.boost.org/doc/libs/1_62_0/doc/html/boost_asio/reference/AsyncReadStream.html), which wraps an existing stream and allows calls to async_read_some to be suspended and resumed.

For the write side you can use a custom body, and suspend by returning boost::indeterminate from the Writer instance: http://vinniefalco.github.io/beast/beast/ref/Writer.html

[georgi-d]

I do not really agree that throttling should be responsibility of the read stream. The whole design of ASIO is to have throttling be done by not issuing new async_* operations. If the reverse proxy was implemented over TCP then we would just have one buffer of fixed length. Then async_read into it, then async_write out of it. Throttling is automatic.

Could you add an example of how a simple reverse proxy would be implemented? The backend server can easily be fixed during construction and not depend on the request.

I feel the implementation would be much simpler by having a custom parser that can be called multiple times than implementing a custom AsyncReadStream.

Thanks

[vinniefalco]

I'll consider pausing the parser during the message body.

What about the Writer solution, does that work for you?

[vinniefalco]

@georgi-d As a temporary solution, you could first read the headers using the headers_parser: https://github.com/vinniefalco/Beast/blob/master/include/beast/http/headers_parser_v1.hpp#L39

And then, construct a new parser for the body using with_body: https://github.com/vinniefalco/Beast/blob/master/include/beast/http/parser_v1.hpp#L286

The resulting parser will be ready to read the message body, and you can just feed in the data. In other words read from the socket yourself and pass the buffers in to parser_v1::write(). When the parser is done, parser_v1::complete() will return true.

The implementation of beast::http::parse is pretty straightforward, there's no magic going on: https://github.com/vinniefalco/Beast/blob/master/include/beast/http/impl/parse.ipp#L220

[georgi-d]

I like the idea of reading the data myself and passing it to the parser with write. I will give it a try.

As for the Writer solution I will try it as well and see how it works for me. I would really like if there is a solution similar to the parser_v1::write for writing the message body so I can control it from the outside rather than being called back for the data.

Thanks

[vinniefalco]

For the write you can specify the type beast::http::empty_body for Body in the message, call beast::http::write to send the headers, and then handle writing of the body yourself, directly on the socket. You won't be able to call prepare (since it will set a zero Content-Length) but that's not a big deal, you can just fill those fields in yourself (you'll also have to set Connection and Transfer-Encoding).

You can still do chunked encoding, Beast provides a routine for you: https://github.com/vinniefalco/Beast/blob/master/include/beast/http/detail/chunk_encode.hpp#L88

I can make chunk_encode part of the public API if necessary.

This would be much easier than defining your own Body and associated writer.

[georgi-d]

This sounds good, I will give it a try. Making chunk_encode public would be useful I believe.

Thanks

[georgi-d]

I started working on reading myself from the socket and passing the data to the parser and ended up copying 90% of async_parse + parse_op. The only extra feature I actually need is the ability to break out of the async_read loop and call the handler on a condition different than parse.complete() returning true.

I was thinking about adding a version of async_parse similar to boost::asio::async_read which receives a CompletionCondition functor. The CompletionCondition should be called after each parser.write() with the number of bytes consumed by the parser and and an error_code (optionally a reference to the parser but it could also be a binded parameter). The default CompletionCondition could be implemented as returning the result of parser.complete().

On error or parser.complete() returning true the handler should also be called.

async_parse() should be callable multiple times for a single http message until the complete message body is consumed and parsed. From I gather the state in parse_op would not need to change to support multiple invocations. On each invocation the data.state variable would start from 0.

Thanks

[vinniefalco]

Why don't you just create your own Parser ? Then you can return whatever value you want from complete(). And you can call async_parse over and over again.

[georgi-d]

I thought about it but feels more like a hack to me. The parser's job is to parse the message and not to determine when the read operation should be completed and the handler invoked. The fact that it currently is used as the sole condition for completion of the async_parse is a special case in my opinion. It is similar to using boost::asio::async_read() with boost::asio::transfer_all() condition. If the parser is used to indicate that the async_read should complete mid message then it should have some other "complete()" to indicate if the message has really ended or not. It should also have a special "reset_read()" method to enable consecutive async_parse () calls.

I will try implementing both solutions to see which one I like better for the use case I have.

[vinniefalco]

@georgi-d It is my intention for parse and async_parse to be something of a "generic read algorithm" that isn't necessarily constrained to only parsing HTTP messages (although thats what the library predominantly uses it for). The parse_op composed operation takes care of some of the heavy lifting for you, so that you can focus on implementing the business logic.

I have been thinking a little bit about renaming async_parse to async_read, rename Parser to Reader, and move the free function and composed operation to <beast/core/read.hpp> since it is now quite general purpose. I don't know that I will actually do this in the near future but that is my thinking.

Given this direction, I don't think you should view using the Parser concept in that fashion as a hack. Beast is a low level library, and exposes building blocks for you to use. There's no "right" or "wrong" way to put together these building blocks. I want the blocks to be flexible, so people can build things with them that I did not anticipate or design for. I think that's the best measure of "success" for this library.

[vinniefalco]

If you wouldn't mind giving me more details about your use-case, perhaps I can improve Beast's interfaces to support it. Or suggest better ways of accomplishing the same result.

[georgi-d]

I have two use cases:

1) High performance HTTP reverse proxy which accepts HTTP requests, parses the headers and based on the path of the request creates a new HTTP to a backend server forwarding the body of the client request to the backend server and the response from the server back to the client asynchronously. The proxy should have fixed memory footprint per connection so it should be able to use a single fixed size buffer per connection and not read the whole message body before forwarding it.

2) A high performance HTTP service which for each HTTP request calls a handler (after parsing the headers) passing it a Request and Response objects.

• The Request should have accessors for the request headers, verb and path and should also have async_read() for reading the request body. The async_read() call should return system::eof when the end of the body is reached.
• The Response object should have setters for the result code and headers and it should also provide async_write() for writing the response body. The Response should have a finish() method for indicating the end of the response. When finish() is called the underlying stream should be returned to the server for waiting and handling a consecutive request from the peer.

Request::async_read() and Response::async_write should be callable multiple times for processing a client request.

I plan on using 2) for implementing 1) and as a transport for a generic REST/RPC framework. The REST/RPC framework allows based on an IDL definition to have client side stubs and server side REST and/or JsonRPC exposed implementation.

[vinniefalco]

This is great, thanks. It sounds like Beast has good interfaces for handling the write side, but we might need to do a little more work on reading. Some questions:

• How will you handle trailing headers?
• How will you handle Expect: 100-continue?
• How do you feel about the current division between message and message_header (which will be renamed to just header), and the routines to write them?

[georgi-d]

• How are trailing headers handled?
• I have not considered trailing headers up to now but I think it would be fine if they appear in the headers container of the message after async_read() returns eof(). I would be fine if they are not supported at all as well.

How is Expect: 100-continue handled?

• on the server side: I think it would be better if handling is explicit. When the handler is called the the code should explicitly check for is_continue_required(request), do any additional checks and do a async_write_response_continue() before doing any async_read() calls.

• on the client side I think it should be handled explicitly as well. If the client adds an Expect: 100-continue header then it should do an explicit async_read_continue() before doing any writes to the request body. The the client should do another async_parse/async_read which should fill any additional headers. Then the response body could be read.

  How do you feel about the current division between message and message_header (which will be renamed to just header), and the routines to write them

• I have not looked too deep into them but I like the separation in general and the presence of the headers_parser which allows to parse the headers, do some handling and then move to parsing the body. On the write side I have not looked much yet. I am still working on the read side. I would like to be able to write the headers separately and then write the body.

[georgi-d]

I have created a draft change which adds CompletionCondition to async_parse():

https://github.com/georgi-d/Beast/commit/c14400711f63b6b1010cd447ace0fce11fb0ada5 For now I have only tested that it compiles. Will write some unit tests next.

[vinniefalco]

I'm not sure that a completion condition is the best way to solve this problem. I need more time to think about it. I believe we can do better.

[vinniefalco]

@georgi-d I've been thinking a lot about this, and I am starting to think that the "stateless" model of the interface is not the right approach. What I mean is that free functions don't encapsulate enough state for us to do useful things. Now I am thinking about a new approach, similar to what is done with websocket:

namespace beast {
namespace http {

template<bool isServer, class NextLayer>
class stream_v1
{
    streambuf rb_;
    NextLayer next_layer_;
    basic_parser_v1 parser_;

public:
    template<class... Args>
    stream_v1(Args&&... args)
        : next_layer_(std::forward<Args>(args)...)
    {
    }

    // Read the header asynchronously
    template<class Fields, class ReadHandler>
    void
    async_read(header<! isServer, Fields>& h, ReadHandler&& handler);

    // Read some of the body asynchronously
    template<class MutableBufferSequence, WriteHandler>
    std::size_t
    async_read_some(MutableBufferSequence const& buffer, WriteHandler&& handler);

    // Write the header asynchronously
    template<class Fields, class WriteHandler>
    void
    async_write(header<! isServer, Fields> const& h, WriteHandler&& handler);

    // Write some of the body asynchronously
    template<class MutableBufferSequence, class WriteHandler>
    std::size_t
    async_write(MutableBufferSequence const& buffer, WriteHandler&& handler);
};

} // http
} // beast

// typical use:
boost::asio::io_service ios;
beast::http::stream<boost::asio::ip::tcp::socket> stream_v1{ios};

This allows us to make the parsing state a member of the class, which persists across member function calls. We would do away with the free functions like http::async_read and replace them with member functions. The parser would always exist, and always have the correct state. This allows us to handle the body differently if we want. It will take me more time to sketch out how this might look in practice. It will require significant modifications to the parser.

We can probably keep the free functions for simple use-cases though.

[vinniefalco]

How do you feel about this:

do_process and do_connect are user-defined:

template<class AsyncServerStream, class AsyncClientStream>
void reverse_proxy(AsyncClientStream& si, yield_context yield)
{
    using namespace boost::asio;
    stream_v1<true, AsyncClientStream> ssi{si};
    // read request header from client
    request_header req;
    ssi.async_read(req, yield);
    // apply request transformation
    do_process(req);
    // establish stream to backend server
    stream_v1<false, AsyncServerStream> sso{true, do_connect(req, yield)};
    // write transformed request to server
    sso.async_write(req, yield);
    // transfer request body to server
    char buf[4096];
    {
        error_code ec;
        for(;;)
        {
            auto const bytes_transferred =
                ssi.async_read_some(buffer(buf, sizeof(buf)), yield[ec]);
            if(ec == error::end_of_message)
                break;
            if(ec)
                throw system_error{ec};
            sso.async_write(buffer(buf, bytes_transferred), yield);
        }
    }
    // read response header from server
    response_header res;
    sso.async_read(res, yield);
    // apply response transformation
    do_process(res);
    // transfer response body to client
    ssi.async_write(res, yield);
    {
        error_code ec;
        for(;;)
        {
            auto const bytes_transferred =
                sso.async_read_some(buffer(buf, sizeof(buf)), yield[ec]);
            if(ec == error::end_of_message)
                break;
            if(ec)
                throw system_error{ec};
            ssi.async_write(buffer(buf, bytes_transferred), yield);
        }
    }
}

[georgi-d]

I like this idea. I just have a few notes:

• we should be able to differentiate when an async_read_some returns eof() because it has reached the end of the body or when it has reached the end of the stream (the socked was closed). This is important to be able to handle multiple requests on the same socket
• after the last async_read_some returns end-of-message it should be able to issue another read for the headers to process the next request on the stream. This should work for pipelined requests as well.
• I would make the parser state a public class/struct
• I would make the asyc_read/write(stream, stream_buf, parser, parser_state, headers) and async_read_some/write(stream, stream_buf, parser, parser_state, buffer) global public methods and then wire stream_v1 to just call them. This will make the implementation more flexible because if needed the state can be managed manually by the application and still achieve the functionality of stream_v1.
• for the trailing headers case it might be needed to pass the headers container to async_read_some as well so it can be updated. Alternatively it could be required to call async_read(headers) after async_read_some returnes end_of_body() to consume the trailers.

[vinniefalco]

• If the socket was closed, you would get error::end_of_message first, and then on the next call to async_read you would immediately get eof.
• Right, it is the intention that reverse_proxy is called in a loop

The problem again is the chunked encoding and trailers. Does async_read_some remove the chunked-encoding for you?

[georgi-d]

I believe that when reading from the stream_v1 the chunk encoding should be removed and only the body data should be available in the buffer.

[vinniefalco]

I believe that when reading from the stream_v1 the chunk encoding should be removed and only the body data should be available in the buffer.

But then we don't know the content length, and the reverse proxy can't forward the body. Look in my code example, the request_header is passed to the upstream connection. If that header comes in with Transfer-Encoding: chunked, then we have to remove that transfer encoding and replace it with Content-Length. But we don't know the content length, because you don't want to read the whole body into memory first.

[georgi-d]

When the request is chunked the reverse proxy should just do its own chunk encoding on the write side. We could add a separate async_read_some_raw() method if we consider re-encoding on the write side a performance bottleneck. The normal functionality should be to remove the chunk encoding on async_read_some() since only a proxy implementation would benefit from not removing the chunk encoding. All normal HTTP server use cases would need the chunk encoding to be stripped.

[vinniefalco]

So, lets say the client sends a one megabyte body broken up into four chunks of 256KB each. Now we forward that to the proxy, but we are using a 4096-byte buffer so the upstream is going to get a body with 256 chunks? That increases the overhead for no good reason, is that okay?

[georgi-d]

If it uses the default behavior I think that would be expected and reasonable. On the other hand now that I think about it the switch whether to strip the chunk encoding or not should be a parameter of the parser and not async_read_some(). So the reverse proxy should set a compile time (template) parameter of the parser that it does not need stripping of the chunk encoding. Then the original chunks can be preserved.

[vinniefalco]

Or, how about the parser gives information about the body in the on_body callback for example:

struct chunk_info
{
    std::size_t chunk_size; // total size of this chunk, or 0 if not chunk-encoded
};

void
on_body(chunk_info& ci, boost::string_ref const& data);

[georgi-d]

I do not like this approach, because it first complicates the API without much benefit and it also would require adding more state on the write side to be able to pass the chunk size beforehand and then fill the chunk with multiple writes (complex and error prone,) or it would require for the proxy to adjust its buffer size to the client's chunk size. This would expose a remote exploit hole because a a malicious client can set a huge chunk size and crash the server with out of memory.

I think the API should be to either chunk encode on the write side to your buffer size or explicitly configure your parser to give you undecoded chunk data when reading and then raw write. The latter is rare and special enough to require a different type (similar to the request/response messages are differentiated) and not a runtime parameter to the parser.

[vinniefalco]

So, if the parser is set to pass-through chunked encoding, it still has to parse the data so it knows where the end of the message is. What do we do about the trailers in that case?

[georgi-d]

Yes, it will still have to parse the data to know where the end of the message is.

For the trailing headers I think the proxy will have to handle them explicitly. It will have to call async_read(trailers) after receiving error::end_of_message if has_trailers(headers) has returned true. Then it would have to write them with async_write(trailers).

[vinniefalco]

So you want to read the trailers into a new object? We can't use request_header or response_header since those have extra information. Are you thinking that we should read the trailers directly into a basic_fields (currently called basic_headers)?

[georgi-d]

According to the spec all trailers should be first listed in the Trailer: header. So there should be a get_trailers(headers) helper which would setup a structure with the expected trailers. Then we should be able to fill the trailers into basic_fields object and validate that all the promised trailers are present. This would mean we would need async_read(expected_trailers, fields).

[georgi-d]

I think it might be worth having error::expect_trailers() or similar error code which is returned after the body has been read and the parser knows that trailers are expected so the application can switch to async_read(trailers).

[vinniefalco]

This is all starting to sound very messy!

[georgi-d]

It is possible to make due without an extra error code. This was just a fruit for thought. I would be fine without it and the application having to check against the headers or the parser. I saw that the parser has a state for expecting trailers.

[georgi-d]

The other general approach is to just make async_read_some receive headers object as parameter so the parser can fill the trailers in if/when they arrive. The error::end_of_message() would be returned after the full body and the trailer has been consumed. This will simplify the state machine the application. For the reverse proxy we would need to be able to write the trailers after writing out the body.

[georgi-d]

It appears that picohttp parser either ignores the trailiers or requires a separate parse_headers call:

phr_decode_chunked(...)
...
/* set consume_trailer to 1 to discard the trailing header, or the application
 * should call phr_parse_headers to parse the trailing header */

[vinniefalco]

@georgi-d Here's an improved version of a function that helps a proxy relay messages. This one handles the trailers. It makes considerable changes to the parser interface. I believe these changes will allow us to improve performance significantly (it borrows techniques from picohttpparser):

/// Efficiently relay one HTTP message between two peers
template<
    bool isRequest,
    class InAsyncStream,
    class OutAsyncStream,
    class MessageTransformation>
void
relay_message(
    InAsyncStream& si,
    OutAsyncStream& so,
    error_code& ec,
    yield_context yield,
    MessageTransformation const& transform)
{
    using namespace beast::http;
    parser_v1<isRequest> p;

    // read the incoming message headers
    for(;;)
    {
        // TODO skip async_read_some if parser already has the next set of headers
        auto const bytes_transferred =
            si.async_read_some(p.prepare(), yield[ec]);
        if(ec)
            return;
        p.commit_header(bytes_transferred, ec);
        if(ec == parse_error::need_more)
        {
            ec = {};
            continue;
        }
        if(ec)
            return;
        break;
    }

    // Create a new message by transforming the input message
    // At minimum this will remove Content-Length and apply Transfer-Encoding: chunked
    auto req = transform(p.get(), ec);
    if(ec)
        return;

    // send the header
    async_write(so, req, yield[ec]);
    if(ec)
        return;

    for(;;)
    {
        // TODO skip async_read_some if we already have body data
        // Read the next part of the body
        auto const bytes_transferred =
            si.async_read_some(p.prepare(), yield[ec]);
        if(ec)
            return;
        p.commit_body(bytes_transferred, ec);
        if(ec == parse_error::end_of_message)
            break;
        if(ec)
            return;

        // Forward this part of the body
        // p.body() removes any chunk encoding
        //
        if(buffer_size(p.body()) > 0)
        {
            async_write(so, chunk_encode(p.body()), yield[ec]);
            if(ec)
                return;
        }
    }

    // Copy promised trailer fields from incoming request
    // that are not already present in the outgoing response:
    //
    for(auto field : token_list{req.fields["Trailer"]})
        if(! req.contains(field))
            req.insert(field, p.get().fields[field]);

    // Send the final chunk, including any promised trailer fields
    //
    streambuf sb;
    write_final_chunk(sb, req);
    async_write(so, sb.data(), yield[ec]);
    if(ec)
        return;
}

[georgi-d]

It looks good to me. I have just a couple of comments:

// Forward this part of the body
        // p.body() removes any chunk encoding
        //
        async_write(so, chunk_encode(p.body()), yield[ec]);
        if(ec)
            return;
    }
   // clear the body from the buffer

I think the body buffer should be cleared explicitly here before looping and reading the next part.

streambuf sb;
    write_final_chunk(sb, req1);
    async_write(so, sb.data(), yield[ec]);
    if(ec)
        return;
}

Can we avoid the use of streambuf here and make write_final_chunk() or add another method make_final_chunk that returns a ConstBufferSequence the same way chunk_encode() does? This is not a blocker just a small optimization.

[vinniefalco]

I think the body buffer should be cleared explicitly here before looping and reading the next part.

The return value of p.body() is invalidated by a subsequent call to prepare(). There's no need to "clear" it, because the storage belongs to the parser. It manages an internal buffer. Note that this is very different from the current implementation, which requires a caller-managed DynamicBuffer object.

Can we avoid the use of streambuf here

You can only avoid streambuf if there are no trailers. This isn't any different from the internal streambuf used when writing the header via async_write(so, req1, yield[ec]);.

This new code solves a few problems from the previous version so I think this is a big step forward. But I am still not completely happy with it yet. It needs a little more polishing I think.

[georgi-d]

I agree. I have a question. What happens to the bytes in the buffer that are part of a consecutive message and are not consumed by the parser? These should be available to a second call to parse headers. I liked the fact that the application could own the buffer it is read into from the socket and the parser was indicating exactly how many were consumed. With the current implementation I feel only the same parser can be used for the next message unless there is a call for the parser to release the buffer and move return it. Why is it needed for the parse to own the buffer? I prefer if the application had more control over the buffer of the body. For the proxy it is not needed but for a normal processing of the body it might be better if it is possible to keep a few chunks until a certain marker is reached and then process and drop them together.

[vinniefalco]

What happens to the bytes in the buffer that are part of a consecutive message and are not consumed by the parser?

I was thinking they could be used in the next call to parse

I liked the fact that the application could own the buffer

If the application owns the buffer, then it forces the library to do an additional allocate/copy for the header fields. If the parser owns the buffer, we can do some significant optimizations. Take a look at how picohttpparser does it: https://github.com/h2o/picohttpparser

[vinniefalco]

I see what you mean though, for the case when there is no chunk-encoding, it makes more sense for the application to be in control of the buffer. There is still more design work to do...

[vinniefalco]

Further improvements. parse_buffer is a new type:

template<class SyncReadStream,
    bool isRequest, class Body, class Fields>
void
read(SyncReadStream& stream, parse_buffer& buffer,
    message<isRequest, Body, Fields>& msg, error_code& ec)
{
    using boost::asio::buffer_copy;
    using boost::asio::buffer_size;

    new_parser_v1<isRequest, Body, Fields> p;

    // Read and parse header
    for(;;)
    {
        p.write(buffer, ec);
        if(! ec)
            break;
        if(ec != error::need_more)
            return;
        ec = {};
        auto const bytes_transferred =
            stream.read_some(buffer, ec);
        if(ec)
            return;
        buffer.commit(bytes_transferred);
    }

    /* At this point one of the following applies:
        1. No body is expected:
            - 100 level response
            - Upgrade request
            - Reading just the header
            - Caller said so
        2. Content-Length is known (no chunked)
        3. Transfer-Encoding: chunked (no Content-Length)
        4. No Content-Length, no chunked
    */
    // Read and parse body
    typename Body::reader r{p.get(), p.content_length()};
    if(p.content_length())
    {
        auto remain = *p.content_length();
        {
            // Copy leftovers in buffer
            auto const n = buffer_size(p.body(buffer));
            if(n > 0)
            {
                buffer_copy(r.prepare(n), p.body(buffer));
                remain -= n;
            }
        }
        while(remain > 0)
        {
            auto const buffers = r.prepare(remain, ec);
            if(ec)
                return;
            auto const bytes_transferred =
                stream.read_some(buffers, ec);
            if(ec)
                return;
            r.commit(bytes_transferred, ec);
            if(ec)
                return;
            remain -= bytes_transferred;
            if(remain == 0)
                break;
        }
    }
    else if(p.chunked())
    {
        for(;;)
        {
            for(;;)
            {
                p.write(buffer, ec);
                if(! ec)
                    break;
                if(ec != error::need_more)
                    return;
                ec = {};
                auto const bytes_transferred =
                    stream.read_some(buffer, ec);
                if(ec)
                    return;
                buffer.commit(bytes_transferred);
            }
            if(p.complete())
                break;
            remain = p.chunk_size();
            // Copy leftovers in buffer
            auto const n = buffer_size(p.body(buffer));
            if(n > 0)
            {
                buffer_copy(r.prepare(n), p.body(buffer));
                remain -= n;
            }
            while(remain > 0)
            {
                auto const buffers = r.prepare(remain, ec);
                if(ec)
                    return;
                auto const bytes_transferred =
                    stream.read_some(buffers, ec);
                if(ec)
                    return;
                r.commit(bytes_transferred, ec);
                if(ec)
                    return;
                remain -= bytes_transferred;
                if(remain == 0)
                    break;
            }
        }
    }
    else
    {
        buffer_copy(r.prepare(buffer_size(
            p.body(buffer))), p.body(buffer));
        remain -= n;
        for(;;)
        {
            auto const buffers =
                r.prepare(r.read_size(), ec);
            if(ec)
                return;
            auto const bytes_transferred =
                stream.read_some(buffers, ec);
            if(ec == boost::asio::error::eof)
            {
                // caller sees eof on the next read
                ec = {};
                break;
            }
            if(ec)
                return;
            r.commit(bytes_transferred, ec);
            if(ec)
                return;
        }
    }
    r.finish(ec);
    if(ec)
        return;
}

[vinniefalco]

Further simplification:

template<class SyncReadStream,
    bool isRequest, class Body, class Fields>
void
read(SyncReadStream& stream, parse_buffer& buffer,
    message<isRequest, Body, Fields>& msg, error_code& ec)
{
    using boost::asio::buffer_copy;
    using boost::asio::buffer_size;

    new_parser_v1<isRequest, Body, Fields> p;

    // Read and parse header
    for(;;)
    {
        p.write(buffer, ec);
        if(! ec)
            break;
        if(ec != error::need_more)
            return;
        ec = {};
        auto const bytes_transferred =
            stream.read_some(buffer, ec);
        if(ec)
            return;
        buffer.commit(bytes_transferred);
    }

    typename Body::reader r{p.get(), p.content_length()};

    // Read and parse body
    while(! p.complete())
    {
        // maybe read chunk delimiter
        for(;;)
        {
            p.write(buffer, ec);
            if(! ec)
                break;
            if(ec != error::need_more)
                return;
            ec = {};
            auto const bytes_transferred =
                stream.read_some(buffer, ec);
            if(ec)
                return;
            buffer.commit(bytes_transferred);
        }
        // copy body bytes in buffer
        p.write_body(r, buffer, ec);
        if(ec)
            return;
        auto const read_size = p.read_size(r, buffer);
        auto const buffers = r.prepare(read_size, ec);
        if(ec)
            return;
        auto const bytes_transferred =
            stream.read_some(buffers, ec);
        if(ec == boost::asio::error::eof)
        {
            ec = {};
            p.write_eof(ec);
        }
        if(ec)
            return;
        r.commit(bytes_transferred, ec);
        if(ec)
            return;
        p.consume_body(bytes_transferred);
    }
    r.finish(ec);
    if(ec)
        return;
}

[georgi-d]

If I understand this correctly it is still trying to read the whole message at once. I do not see from here how one can control how much of the body/headers are read in one call.

What concept/interface do parse_buffer and Body have?

[georgi-d]

I feel the state machine is too complicated and the parser interface currently does not give enough control over the body buffer. The concept of Body also feels a bit over complicated.

After some consideration I thnk the parser should be much simpler and have members only for its own internal state.

The parser and the methods to read the full message should look like this:

template <bool IsRequest, bool ChunkDecodeBody = true>
class parser_v1 {

   /*
    Parses an HTTP message filling the fields and the body buffer

    @param buffer [in] raw http buffer to read from
    @param fields [in/out] container to add the headers, trailers and verb/status
    @param body [out] buffer to store any parsed body data
    @param ec [out] error::need_more if more input is needed to complete the message
                    error::have_more if the body buffer is full and there is more data in the input
                    parse error 

    @return pair containing bytes_consumed and body_bytes produced
   */
   template <class ConstBufferSequence, Fields, MutableBufferSequene>
   pair<size_t, size_t>
   write(const ConstBufferSequence& buffer, Fields& fields, MutableBufferSequene& body, error_code& ec);

   /*
    Parses an HTTP message headers only filling the fields

    @param buffer [in] raw http buffer to read from
    @param fields [in/out] container to add the headers, trailers and verb/status
    @param body [out] buffer to store any parsed body data
    @param ec [out] error code if error is encountered or error::need_more if the end of the headers have not been reached

    @return pair containing bytes_consumed and 0
   */
   template <class ConstBufferSequence, class Fields>
   pair<size_t, size_t> 
   write(const ConstBufferSequence& buffer, Fields& fields, error_code& ec);

   /*
   Returns whether the complete message has been parsed
   */
   bool complete();
};

/*
Read and parse the HTTP headers
*/
template<class SyncStream, class Parser, class ReadDynamicBufferSequence, class Fields>
void
read_headers(SyncStream& stream, Parser& parser, ReadDynamicBufferSequence& read_buf, Fields& fields, error_code& ec)
{
   for (auto bp = parser.write(read_buf.data(), fields, ec), readbuf.consume(bp.first);
        !parser.complete() && ec == error::need_more();
        bt = parser.write(read_buf.data(), fields, ec), readbuf.consume(bp.first))
   {
      auto const rs = read_size(read_buf);
      ec = {};
      const auto br = stream.read_some(read_buf.prepare(rs), ec);
      read_buf.commit(br);
      if (ec)
      {
         return;
      }
   }
}

/*
Read a full HTTP message filling the body into a stream_buf
TODO: Add support for CompletionCondition
*/
template<class SyncStream, class Parser, class ReadDynamicBufferSequence, class Fields, class BodyDynamicBufferSequence>
void
read(SyncStream& stream, Parser& parser, ReadDynamicBufferSequence& read_buf, BodyDynamicBufferSequence& body, error_code& ec)
{
   read_headers(stream, parser, read_buf, fields, ec);

   if (ec)
   {
      return;
   }

   if (continue_required(fields))
   {
      write_continue(stream);
   }

   const auto ws = prepare_size(body);

   for (auto bp = parser.write(read_buf.data(), fields, body.prepare(ws) ec), readbuf.consume(bp.first), body.commit(bp.second);
        !parser.complete() && (ec == error::need_more() || ec == error::have_more());
        bt = parser.write(read_buf.data(), fields, ec), readbuf.consume(bp.first), body.commit(bp.second))
   {
      if (ec == error::need_more())
      {
         auto const rs = prepare_size(read_buf);
         ec = {};
         const auto br = stream.read_some(read_buf.prepare(rs), ec);
         read_buf.commit(br);
         if (ec)
         {
            return;
         }
      }
   }
}

On top of the simple parser interface it is very easy to expose a http_stream_v1 implementation which holds reference to (parser, fields, stream_buf) and allows to read the body into a MutableBuffer. The http stram would return eof() when the end of the message is reached.

This design also allows operation only with static buffers if needed.

[vinniefalco]

I want to make the parser operate on a linear buffer instead of a BufferSequence. There are significant optimizations that can be performed if the entire header is in a single piece of memory. There's a trade-off here, because you have to either realloc if you don't have enough of the header, or you need to pre-allocate a larger than necessary buffer. However, I believe that its worth it. picohttpparser (https://github.com/h2o/picohttpparser) outperforms the Node.JS parser that Beast is modeled after, by 1000% (!).

Having the whole header in a single buffer also allows for better parsing of the URI in request messages. I can add a parser to break down the URI into its scheme, authority, userinfo, path, and query parameters. This is a significant improvement that users have requested.

A single buffer also allows efficient handling of continuation lines (they can be combined in place). And it allows for better treatment of chunk extensions (currently unimplemented in Beast).

I appreciate that you have taken the time to propose a design for the parser. One thing I notice in your interface, it requires buffer copying to process the body. In the model I am trying to build, the read function transfers data directly from the socket into the container used to store the body, with no intermediate buffer sequence (except for a small constant number of leftover bytes from the linear buffer which must be copied).

I agree that the current Reader concept is likely overcomplicated. However, it still needs to exist to allow callers to customize the type of the message::body member (and still be able to read it in).

As for the complexity of the parser I am trying to build, I agree that it is not exactly simple. Hopefully I can simplify it some. But HTTP is not a simple protocol so I think there are limits to how simple it can be, while also allowing a maximally performant implementation. I'm okay with some complexity as long as it enables all the use cases that people might want. Beast intends to be a low level library (for HTTP). The design goals are flexibility, performance, and simplicity in that order.

I'm still working on a prototype of the things we've talked about, this is where I'm at: https://github.com/vinniefalco/Beast/blob/http/test/http/new_parser.cpp It is still a work in progress.