Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP server should allow a handler using request content streaming to not read an empty content #907

Closed
nyh opened this issue May 23, 2021 · 0 comments
Labels

Comments

@nyh
Copy link
Contributor

nyh commented May 23, 2021

In commit 4fa0e4b, we (@wmitros) added the ability of an HTTP handler to read the request's body using an input stream instead of as a pre-read string. During the review of this series, we had long discussions what to do when a handler doesn't read the entire request body. The decision was in this case to close the connection after sending the response - as it can no longer be reused by the client because the server didn't fully read the previous request.

However, to test whether the handler read the entire request body we check: (httpd.cc)

if (!content_stream.eof()) {
    _done = true;
}

There is a caveat with this eof() check: It is only guaranteed to return true after a read() on this stream returned nothing. But consider a case where simpler handler doesn't expect any body at all (e.g., a trivial GET request) so it doesn't bother to read the body at all. In this case eof() may return false (and we saw this in practice in the Scylla project - scylladb/scylladb#8691), and the connection will be closed although there is no real reason to do this.

So I propose changing the above code to the following almost-as-simple code:

return content_stream.read().then([this] (temporary_buffer<char> buf) {
    if (!buf.empty()) {
        _done = true;
    }
});

Basically this tries a read(), which either returns whatever data is already available in the content stream, or returns nothing if there is nothing left in the content stream. If the content stream has zero bytes - it will know this and not close the connection.

Note: although read() may block, we expect it to usually return immediate (giving either an EOF or some previously read data) and, in rare cases, wait until the client continue to send more data we're not planning to read. In any case, it can't simply hang waiting for data which will never arrive, because we know (via either content-length or chunked encoding) how much data the client is planning to send, and beyond that we will get an EOF immediately from the content stream.

@wmitros what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant