preventing hang when TCPSocket readable, but no app-data is available

[dotconfig404]

eof? is a blocking read that times out when no application data is available - it should not be used as a readiness probe

[dotconfig404]

fixes #233

[rhenium]

Would trying read_nonblock once be enough?

This should remain in the else clause, as a new connection doesn't need a liveness check.

[rhenium]

I wonder if we should check for unclean disconnections (Errno::ECONNRESET/SystemCallError and OpenSSL::SSL::SSLError raised by read_nonblock), not just EOF.

ruby/ruby#1089 suggested it, but the patch wasn't merged and the eof? check was introduced instead because of a compatibility issue with Windows. I don't think the issue exists for sockets anymore.

[dotconfig404]

Hi, thank you for the review!

Changed it to be in the else clause.

The loop's intent was to drain the connection completely of all of the potential non-application data, but that is being done anyways since SSL_MODE_AUTO_RETRY is on, so yes, it's not necessary. Changed that as well.

Currently it calls a eof? which calls this:

  def fill_rbuff
    begin
      @rbuffer.append_as_bytes(self.sysread(BLOCK_SIZE))
    rescue Errno::EAGAIN
      retry
    rescue EOFError
      @eof = true
    end
  end

So at the moment we have two error cases that are being checked:

1. EOFError, which is handled via == nil check
2. EAGAIN, which is the equivalent of SSLErrorWaitReadable/SSLErrorWaitWritable, or rather the symbols wait_readable/ :wait_writable being returned, and these aren't really errors or show that the connection needs to be reset.

So it appears we would actually extend error handling, since the behavior besides blocking vs nonblocking stays the same. Thus I don't believe it's necessary to check for that.

Besides, there is some error handling for these here

[dotconfig404]

Actually it seems we are back to the suggested PR from:
ruby/ruby#6423

almost the same, except that it uses read_nonblock(0) instead of read_nonblock(1) which would actually never result in detecting EOFError because no SSL_read is executed due to this and this.

If the latter check was gone we could implement a separate function to call a nonblocking read of maxlen=0, that'd be suitable for "flushing" the TCPSocket whilst potentially loading application data in the OpenSSL buffer, instead of taking it out and not consuming, but probably not necessary...

Just feels a bit bad to call read of length 1 and just expect it will never return data. I suspect it's more of an issue with how transports are being done in the first place though.

[dotconfig404]

btw, the aforementioned PR also included a check if the socket is a SSLSocket due to some failing spec (if defined?(OpenSSL::SSL) && @socket.io.is_a?(OpenSSL::SSL::SSLSocket)), but it seems this is not necessary anymore.

I ran the spec suite with the change from this PR (rebased to tag v0.6.0 to prevent other unrelated faiures due to some breaking changes in the new 0.7.0), and ruby 3.4.7 without failures.

[rhenium]

Besides, there is some error handling for these here

An exception in #begin_transport would be caught there, and the request would be retried on a new connection, but only if the HTTP method is idempotent. However, it should be safe to retry the request regardless of the method, since it hasn't actually been sent yet. That said, this might be a separate topic.

This PR looks good to me, but I've requested a review from the maintainer of net/http (@nurse).

Trying to read 1 byte should be safe, assuming a server isn't allowed to return a response before receiving a request, though the current HTTP/1.1 RFC https://datatracker.ietf.org/doc/html/rfc9112 doesn't appear to state it explicitly.

If we do support such servers, we might want to extend @socket (Net::BufferedIO from https://github.com/ruby/net-protocol/) so that the 1 byte won't be lost. It maintains another read buffer on top of TCPSocket/SSLSocket.

[rhenium]

As for OpenSSL::SSL::SSLSocket#read* with 0 argument, the behavior is the same as IO#read* which also doesn't reach read(2) and returns an empty string at EOF.

[dotconfig404]

Thanks! Right, net-protocol is probably a good place to extend that if needed eventually