Skip to content

Commit

Permalink
httpd: allow handler to not read an empty content
Browse files Browse the repository at this point in the history
Starting in commit 4fa0e4b, an HTTP handler
can read a request's body using an input stream instead of as a string.
If the handler does not read the entire request body, the client can not
send any further requests on this socket, so the server needs to close it.
Conversely, if the handler *did* read the content, the server may keep the
socket open.

To check whether the handler read the content we checked the content
stream's eof() state. However, this may only become true after a read()
noticed the content has reached its end (specified by content-length or
chunked encoding). But one interesting case we missed is a simple handler
which assumes an empty request body, and doesn't bother to read it at all.
In this case, even though the request is empty, and the handler didn't
miss any content by not reading it, we close the connection.

The solution in this patch is to replace the check for eof() by an attempt
to read() from the content stream. In the typical case the handler either
read the entire content or there was nothing to read in the first place,
so this read() will return nothing immediately - and we allow reusing the
connection. If anything is returned by the read(), it is ignored, and the
connection is closed (we may still not read the entire content).

Fixes #907
Refs scylladb/scylladb#8691

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20210523184038.425954-1-nyh@scylladb.com>
  • Loading branch information
nyh authored and avikivity committed May 24, 2021
1 parent ec0318f commit f0f28d0
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions src/http/httpd.cc
Original file line number Diff line number Diff line change
Expand Up @@ -279,12 +279,16 @@ future<> connection::read_one() {
return generate_reply(std::move(req));
}).then([this, &content_stream](bool done) {
_done = done;

// If the handler did not read the entire request content, this connection cannot be
// reused so we need to close it. "_done = true" does that.
if (!content_stream.eof()) {
_done = true;
}
// If the handler did not read the entire request
// content, this connection cannot be reused so we
// need to close it (via "_done = true"). But we can't
// just check content_stream.eof(): It may only become
// true after read(). Issue #907.
return content_stream.read().then([this] (temporary_buffer<char> buf) {
if (!buf.empty()) {
_done = true;
}
});
});
}).handle_exception_type([this, &version] (const base_exception& e) mutable {
// If the request had a "Transfer-Encoding: chunked" header and content streaming wasn't enabled, we might have failed
Expand Down

0 comments on commit f0f28d0

Please sign in to comment.