Ensure 1xx and Remainder responses mark connection as not persistent

[zarqman]

This PR ensures that responses with either 1xx statuses or Remainder bodies both set @persistent = false.

1xx statuses indicate that the current response is not complete. Similarly, Remainder bodies indicate an indeterminate body and thus an inability to know when it is complete. In both cases, this makes it important to mark the client connection as not persistent.

This is required so that Async::Pool::Controller will not accidentally attempt to reuse the connection. When determining whether a connection is eligible for reuse, Pool calls connection.reusable?, which in turn references @persistent.

The choice to modify @persistent inside read_remainder_body was chosen as it parallels similar behavior in various write_* methods. I also considered adding logic to #persistent? to check status. While that would work for 1xx statuses, there are also other instances of returning a Remainder body (via #read_remainder_body), some of which cannot reasonably be checked inside #persistent?.

This PR changes 1xx responses to return a Remainder instead of nil. Returning nil previously seemed like a bug in its own right, as there is definitely more on the stream and returning nil is misleading. If the Remainder body is unused, as it would be when a websockets connection hijacks the connection, there is no harm. However, if websockets is unused or if it fails to hijack the connection (perhaps because the WS headers are incorrect), this ensures any following client code sees a body and not nil.

I tested this against async-websockets and tests still pass with body as a Remainder.

Related, any response with body=nil is ineligible to be wrapped, whether by Statistics, Client's deferred checkin for Pool::Controller, or otherwise. In the case of Pool, this can cause a premature checkin, which can potentially lead to reuse of a connection before the current client is finished with it. Failing to wrap responses because of a nil body is a separate issue, but I wanted to solve this in a manner congruent with improving that overall situation by assigning an actual Body.

Lastly, since async-http recently added handling of 1xx statuses (yay!), two additional thoughts:

First, when Request.read loops on read_response due to 1xx statuses, read_response will recalculate @persistent= on each iteration. This works great and will ensure that as long as the connection eventually ends in a final status (other than 101), the connection ends as persistent/reusable. Even though considered final by protocol-http, 101's will be marked as not persistent, again proper since the connection cannot be reused for http once the protocol has been upgraded (to websockets or otherwise).

Second, the changes here make protocol-http1 and persistent do the right thing independent of async-http. That is, 1xx responses aren't relying on the looping behavior of async-http to be safe.

Types of Changes

• Bug fix.

Contribution

• I added tests for my changes.
• I tested my changes locally.
• I agree to the Developer's Certificate of Origin 1.1.

[ioquatix]

This looks okay to me but let me review it in detail.

[ioquatix]

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.

[zarqman]

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.

[ioquatix]

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

[zarqman]

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.

[ioquatix]

I'm considering this approach again. I think I agree with the general idea.

[ioquatix]

I will experiment with this change. My primary interest is proxying upgrade requests correctly and I think this is a step in the right direction.

Thanks for your contribution and your detailed discussion, I really appreciate it, and apologies for taking time to come round to the idea.