Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/protocol/http1/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ def read_fixed_body(length)
end

def read_remainder_body
@persistent = false
Body::Remainder.new(@stream)
end

Expand Down Expand Up @@ -469,7 +470,14 @@ def read_response_body(method, status, headers)
return nil
end

if (status >= 100 and status < 200) or status == 204 or status == 304
if status >= 100 and status < 200
Copy link
Member

@ioquatix ioquatix Oct 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should handle 101 here explicitly? i.e. I don't think 100 continue should respond with a body, and it's also true that 101 affects connection persistence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of my goals here is to ensure that all 1xx responses are default-safe whether or not 1xx is looped on (via async-http's Request.read) and whether or not 101 ends up being used for a websocket connection.

The most important part is ensuring @persistent=false happens for all 1xx (including 101). That could be done here without the Remainder. Failing to do at least this much opens up a potential desync.

As I tried to mention in the OP, I'm hoping to move toward every Response having a body. I've been testing this in my own code with great success as it solves issues like Statistics and other wrappers not being able to wrap responses that lack bodies. In my own case, that includes wrapping 101's. Converting non-1xx to all have bodies is obviously a larger discussion and beyond the context here.

Returning a Remainder here has some benefits:

a) For 101 with Upgrade: websocket, if Async::HTTP::Client or another non-websocket-aware client is in use, then Remainder hints that there is something remaining on the stream (which indeed there is). More, any call to response.close would properly close that stream and not leave it dangling open (which is the case when response.body==nil).

b) For 101 with Upgrade: <other-than-websocket>, the Remainder is directly usable. Depending on the target protocol, that is potentially useful. And, the stream can still be hijacked for more direct access to the underlying socket (and the Remainder simply ignored).

c) For all 1xx, the Remainder can be thought of as a type of promise on the future data on the socket. Again, this hints to a developer that there's more to come. But it's more than a hint--it also clarifies that this socket is not available to be reused yet because the stream of data is not complete. To me, nil implies that there is nothing more coming.

Helpfully, using a Remainder for all 1xx shouldn't be detrimental to existing use since 1xx support is newly added. The only existing usage was via async-websocket, and anything that hijacks the connection was already ignoring the nil body, so it will simply ignore the Remainder body now. Barring something pretty weird, I believe this to be a non-breaking change.

All that said, if you're really uncomfortable with changing to Remainder, I can rework it to maintain nil and simply set @persistent instead.

# At the moment this is returned, the Remainder represents any
# future response on the stream. The Remainder may be used directly
# or discarded, or read_response may be called again.
return read_remainder_body
end

if status == 204 or status == 304
return nil
end

Expand Down
5 changes: 4 additions & 1 deletion test/protocol/http1/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,9 @@
with '#read_response_body' do
with "GET" do
it "should ignore body for informational responses" do
expect(client.read_response_body("GET", 100, {'content-length' => '10'})).to be_nil
body = client.read_response_body("GET", 100, {'content-length' => '10'})
expect(body).to be_a(::Protocol::HTTP1::Body::Remainder)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we used this body in any way, wouldn't it corrupt the underlying connection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no. While not a dominant use case, one could read from the Remainder and it would simply end up returning the next set(s) of response headers followed by the body.

Think of Remainder in this context as a promise against the rest of the stream.

expect(client.persistent).to be == false
end

it "should ignore body for no content responses" do
Expand All @@ -214,6 +216,7 @@
it "should handle non-chunked transfer-encoding" do
body = client.read_response_body("GET", 200, {'transfer-encoding' => ['identity']})
expect(body).to be_a(::Protocol::HTTP1::Body::Remainder)
expect(client.persistent).to be == false
end

it "should be an error if both transfer-encoding and content-length is set" do
Expand Down
2 changes: 1 addition & 1 deletion test/protocol/http1/hijack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
expect(headers).to have_keys(
'upgrade' => be == ['websocket'],
)
expect(body).to be_nil # due to 101 status
expect(body).to be_a(::Protocol::HTTP1::Body::Remainder) # due to 101 status

client_stream = client.hijack!

Expand Down