Removed inappropriate $! reference in Net::FTP

[gotoyuzo]

$! in ensure section may be an Exception object that already exists before entering its begin...end.
Net::FTP#transfercmd uses $! for error handling, so using Net::FTP in rescue section will cause an unexpected data connection drop. Since an exceptions is required to decide to close the data connection, it should be placed in rescue section.

The reproduction code is as follows:

require "net/ftp"

begin
  raise "FOO!"
rescue
  ftp = Net::FTP.new("ftp.example.com")  # specify an anonymous FTP server
  ftp.login
  p ftp.list
end

result:

$ ruby ftp-err.rb
/home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/3.2.0/socket.rb:456:in `__read_nonblock': closed stream (IOError)
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/3.2.0/socket.rb:456:in `read_nonblock'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-protocol-0.2.2/lib/net/protocol.rb:218:in `rbuf_fill'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-protocol-0.2.2/lib/net/protocol.rb:199:in `readuntil'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-ftp-0.2.0/lib/net/ftp.rb:1510:in `gets'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-ftp-0.2.0/lib/net/ftp.rb:690:in `block (2 levels) in retrlines'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-ftp-0.2.0/lib/net/ftp.rb:335:in `with_binary'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-ftp-0.2.0/lib/net/ftp.rb:687:in `block in retrlines'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/3.2.0/monitor.rb:202:in `synchronize'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/3.2.0/monitor.rb:202:in `mon_synchronize'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-ftp-0.2.0/lib/net/ftp.rb:686:in `retrlines'
        from /home/gotoyuzo/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/net-ftp-0.2.0/lib/net/ftp.rb:944:in `list'
        from ftp-err.rb:8:in `rescue in <main>'
        from ftp-err.rb:3:in `<main>'
ftp-err.rb:4:in `<main>': FOO! (RuntimeError)

[shugo]

Thank you!
$! is not a local variable but a thread-local variable, so it's unsafe to use $! outside rescue clauses.

@jeremyevans I've merged it, but tell me if something is wrong.

[jeremyevans]

@shugo it looks fine, except that it will not close the connection for non-StandardError exceptions. Maybe rescue Exception? We are reraising right after closing the connection, so it should be safe.

[shugo]

@jeremyevans Oh, I'll fix it. Thanks!

[ioquatix]

I don't believe the merged fix is correct.

There are flow control primitives like Thread#kill, throw and break/next which won't execute the rescue block. Is that okay?

See also this discussion: https://bugs.ruby-lang.org/issues/18083

[jeremyevans]

We could use a construction such as:

begin
  # ...
rescue Exception => e
  raise
ensure
  conn&.close if e
end

@ioquatix do you think that is the best approach?

[shugo]

We could use a construction such as:

begin
  # ...
rescue Exception => e
  raise
ensure
  conn&.close if e
end

There may be still a race codition when the thread is killed.

t = Thread.start {
  begin
    sleep
  rescue Exception => e
    raise
  ensure
    p e
  end
}
sleep(0.1)
t.kill

How about the following code?

succeeded = false
begin
  # ...
  succeeded = true
ensure
  conn&.close if !succeeded
end

[jeremyevans]

The approach without rescue is fine if the ... part doesn't yield or otherwise involve calling user code. If it can call user code, the approach will close the connection for user-initiated non-local jumps (return inside a block or throw), which is not what you want if you only want to close when an exception is raised.

You can use conn&.close if e || Thread.current.status == 'aborting' in my example to handle the Thread#kill case.

[ioquatix]

In https://bugs.ruby-lang.org/issues/15567 I discussed these types of problems, and I don't think there is any easy solution because not enough information is exposed.

However, I think the proposed:

succeeded = false
begin
  # ...
  succeeded = true
ensure
  conn&.close if !succeeded
end

Is quite good.

Ultimately what we want is something like:

begin
  # ...
ensure when not return
  conn&.close
end

I don't think we should implement some elaborate mechanism which checks the thread status etc. As I think that is a code smell. This is a common pattern and it should be easy to do the correct thing.

[ioquatix]

(There is also an argument to be made that we shouldn't try to handle Thread#kill).

[shugo]

I've created a pull request: #27

It's may not be a perfect solution, but I also prefer the solution by the succeeded flag.

(There is also an argument to be made that we shouldn't try to handle Thread#kill).

The solution by the succeeded flag doesn't handle Thread#kill explicitly, so I prefer it over checking Thread.current.status.