do not wait for requests if the ssl socket already buffered bytes

[HoneyryderChuck]

A condition may happen where a socket may have have already drained the bytes to the TLS layer; this would cause this particular "keep alive while waiting for next request" hanging, despite the client having sent the request already, because the wait_readable call is performed on the TCPSocket, not on the SSLSocket (which anyway, I don't think the implementations check pending bytes either).

I get this behaviour frequently when running running httpx tests in jruby; something about it and the JVM thread scheduling makes this edge case happen more often under heavy thread contention (httpx tests run in parallel mode).

One argument could be made that this should be fixed at the openssl libs CRuby openssl doesn't check pending bytes in wait_readable either, but the pattern of doing socket.to_io.wait_readable is quite popular, so I imagine it'd take years to catch up to that.

LMK what you think.

[jeremyevans]

Not saying we shouldn't fix this, but I highly recommend switching from webrick to puma for the httpx tests.

[rhenium]

One argument could be made that this should be fixed at the openssl libs CRuby openssl doesn't check pending bytes in wait_readable either, but the pattern of doing socket.to_io.wait_readable is quite popular, so I imagine it'd take years to catch up to that.

The correct link for ruby/openssl's wait_readable would be https://github.com/ruby/openssl/blob/ed948eae356b4226a10e59d772fdbbb845aecae2/lib/openssl/ssl.rb#L230-L232. It currently doesn't check for pending data, either in OpenSSL::Buffering or in TLS records internally buffered by OpenSSL. This seems like a bug to me.

Please note OpenSSL::SSLSocket#pending still doesn't account for OpenSSL::Buffering's read buffer, at least in ruby/openssl. I think a more reliable way to check readability here is to actually try to read from the socket with sock.read_nonblock(1) and unget the byte if the read succeeds.

[HoneyryderChuck]

@jeremyevans puma is unfortunately an option. The tests handstitch its own test servers, and puma does not make it particularly easy to start an instance from ruby (it targets rack, config.ru's, etc preferably). besides, I rely a lot on the servlet architecture of webrick, as well as some specific functionality, like the http proxy mode, or the digest auth module, which puma doesn't have and probably ain't interested in backporting. webrick is a fine tool as is, even with all its known production limitations.

It currently doesn't check for pending data, either in OpenSSL::Buffering or in TLS records internally buffered by OpenSSL. This seems like a bug to me.

Perhaps it is, but as stated above, a lot of tools in the wild use sock.to_io.wait_readable (I skimmed across other http clients, and net-http and httprb do it. SSLSocket#pending is also public API, which can be used today (for workarounds like the one from this PR), as ecosystem migrates.

Please note OpenSSL::SSLSocket#pending still doesn't account for OpenSSL::Buffering's read buffer, at least in ruby/openssl. I think a more reliable way to check readability here is to actually try to read from the socket with sock.read_nonblock(1) and unget the byte if the read succeeds.

I'd say it's preferable to fix the openssl bug than building the workaround you're suggesting, just because this issue isn't as prevalent in cruby (something about the thread scheduler makes it happen almost never?). JRuby's the one where this needs to be correct, as workarounds go.

[rhenium]

I spoke too soon about OpenSSL::SSL::SSLSocket#wait_readable. I think we probably can't check for the buffers in it. I've opened a PR at ruby/openssl to clarify docs because I find this confusing: ruby/openssl#1119

I suggested using #read_nonblock because that works today and removes the need to check for the SSLSocket-specific method, though I agree it would look awkward. WEBrick::HTTPRequest appears to be using SSLSocket#gets, so SSLSocket may potentially contain some buffered data that SSLSocket#pending doesn't report. I've not actually try to reproduce this, but it seems possible to me even with CRuby.