redis / hiredis-rb

Ruby wrapper for hiredis
BSD 3-Clause "New" or "Revised" License
319 stars 90 forks source link

Set autoclose: false on socket to prevent GC from closing it #82

Closed stanhu closed 2 years ago

stanhu commented 2 years ago

This updates #59 with the latest master.

Hiredis::Ext::Connection#sock creates a Socket object using Socket.for_fd, wrapping the socket managed in C by hiredis and returns it to the caller. The Socket does not have autoclose set to false, so this has the unwanted side-effect that once the object becomes stale and is GC'd, the underlying file descriptor will be closed. This can cause Errno::EBADF errors, as outlined in Ruby Bug #1174, especially when the Redis driver performs a reconnect and creates a new instance of Hiredis::Ext::Connection, forgetting the old one completely.

Note that Errno::EBADF errors might affect other parts of the program, as the FD is closed at GC time and it may refer to something else. The following script reproduces the issue:

  require 'redis'

  # Open /dev/null as a placeholder
  devnull = File.open("/dev/null")

  r = Redis.new(driver: :hiredis)
  r.ping

  # Close /dev/null to create a hole in the FD table
  devnull.close

  # Obtain a reference to the hiredis socket; this leaks the socket to Ruby space
  # and will close the underlying FD on GC
  con = r.client.connection
  sock = con.instance_variable_get(:@connection).sock
  puts "Redis connection on FD #{sock.fileno}"

  # Recycle the redis connection, using the hole left by /dev/null
  r.client.disconnect
  r.ping
  puts "Reconnected Redis"

  # Open /dev/null again; normally this will occupy the old redis socket FD
  devnull = File.open("/dev/null")
  puts "Opened /dev/null on FD #{devnull.fileno}"

  # Forget the reference to make the socket GC'able
  sock = nil

  # Trigger a GC run
  puts "Triggering a GC run"
  GC.start

  # Try to read /dev/null
  puts "Reading from /dev/null"
  devnull.read

Fix this by setting autoclose to false in the created Socket object.

stanhu commented 2 years ago

@michael-grunder Would you be able to trigger CI, merge this, and release a new hiredis-rb version?

I'm also happy to help maintain these releases.

michael-grunder commented 2 years ago

Merged, thanks.

I'll put together a release, and I'll see about getting you added as a maintainer if that works for you.