Consistenty - `IOError` vs `EBADF`?

[ioquatix]

SSLSocket error conditions are not consistent with IO.

1. io.close followed by io.read can result in EBADF rather than IOError.
2. Without sync_close, io.close followed by io.read will hang. Even if the underlying IO is not closed, I don't think the SSLSocket instance should continue to work after being closed?

Reproduction:

#!/usr/bin/env ruby

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'localhost'
end

require 'socket'
require 'openssl'
require 'localhost'

def read_after_close(io)
  io.close
  io.read
end

begin
  client, server = Socket.pair(:UNIX, :STREAM)
  read_after_close(client)
rescue => error
  $stderr.puts error.full_message
end

begin
  authority = Localhost::Authority.fetch
  
  client, server = Socket.pair(:UNIX, :STREAM)
  
  ssl_server = OpenSSL::SSL::SSLSocket.new(server, authority.server_context)
  ssl_server.sync_close = true
  
  ssl_client = OpenSSL::SSL::SSLSocket.new(client, authority.client_context)
  ssl_client.sync_close = true # If this is not set, `io.read` above will hang which is also a bit odd.
  
  Thread.new{ssl_server.accept}
  
  ssl_client.connect
  
  read_after_close(ssl_client)
rescue => error
  $stderr.puts error.full_message
end

Is this something we can improve?

[rhenium]

Calling read/write on a closed socket is a user error, but hanging indefinitely doesn't seem ideal. I think we can manage a flag in SSLSocket so we can raise IOError.

What should happen if a thread closes SSLSocket without sync_close while another thread is waiting for rb_io_maybe_wait_readable() on the underlying socket?

[ioquatix]

I'm on the fence about how this should work.

On the one hand, I understand someone might call io.close. What happens to other users blocking io.read?

There are two options:

1. io.close is able to cause other operations to abort io.read etc. However, there are many such operations, and so this implementation is extremely complex. It's current behaviour (does it have race conditions? not sure).
2. io.close with pending operations fails as a runtime error. In other words, io.close can fail if you don't cause all other usage to exit first. The implementation of this is more trivial as it's just a reference count.
3. Maybe as a middle ground, io.close should simply wait for all operations to finish. In other words, it blocks indefinitely if the io is in use elsewhere.

As we see in my example here, and your response about how should sync_close be handled - it becomes extremely tricky.

@ko1 do you have any thoughts about it? I know you believe that File.open{...} should be able to close the file in all situations, but I'm not so sure if it's good. If the user writes this

File.open(..) do |io|
  Thread.new{io.read}
end

I also believe this is a problem with the program.

[ioquatix]

For consistency's sake, I think we should fix this. IO#read_nonblock (raises IOError) and SSLSocket#read_nonblock (raises Errno::EBADF) seem inconsistent in this regard.

[ioquatix]

See socketry/io-stream#6 for a related PR that is working around this issue.