attached IO must be closed when its file descripter is closed #200

Closed
mame opened this Issue May 12, 2011 · 11 comments

Comments

Projects
None yet
5 participants

mame commented May 12, 2011

Hello,

An IO object attached by EM.attach seems not to be closed
even if the corresponding Connection#close_connection is
called and its file descriptor is actually closed:

require "eventmachine"
r, w = IO.pipe
EM.run do
  conn = EM.attach(r)
  conn.close_connection
  EM.stop
end
p r.closed?  #=> false (true expected)
r.close      #=> Bad file descriptor (Errno::EBADF)

eventmachine must not leave IO open when it closes its
file descriptor.
This causes an actual problem because the descriptor
may be reused. If the file descriptor is reused (by
creating a new IO, for example), and then if the IO is
collected by GC, the descriptor will be closed wrongly.

I attach a workaround patch to ensure the IO is closed.
But eventmachine may be still wrong if the descriptor
is reused after the descriptor is closed and before IO
is closed. Formally, eventmachine should not directly
close Ruby IO's file descriptor by close(2), but should
close it using Ruby's IO#close.

diff --git a/lib/eventmachine.rb b/lib/eventmachine.rb
index d6517a4..0b6098e 100644
--- a/lib/eventmachine.rb
+++ b/lib/eventmachine.rb
@@ -1395,6 +1395,11 @@ module EventMachine
           else
             c.unbind
           end
+          io = c.instance_variable_get(:@io)
+          begin
+            io.close
+          rescue Errno::EBADF
+          end
         rescue
           @wrapped_exception = $!
           stop

Thanks,

Yusuke Endoh mame@tsg.ne.jp

dv commented Aug 6, 2011

I would also like to see this issue resolved since I'm using #attach pretty heavily in my application.

dv commented Aug 6, 2011

Here's a possible workaround: disable autoclose for the io:

require "eventmachine"
r, w = IO.pipe
r.autoclose = false
EM.run do
  conn = EM.attach(r)
  conn.close_connection
  EM.stop
end
p r.closed?  #=> false (true expected)
r.close      #=> Nothing.

This way the IO finalizer won't call close() during garbage collection.

Contributor

tmm1 commented Aug 6, 2011

Good find. IO#autoclose=false should fix this issue if you're using ruby 1.9.x

This issue is blocking blather's file transfer feature. Can this fix be included for 1.0?

Contributor

tmm1 commented Aug 13, 2011

I can include the autoclose fix, but it will only work on 1.9.

dgraham commented Aug 13, 2011

Here's the Blather code that this bug is affecting:
https://github.com/sprsquish/blather/blob/master/lib/blather/file_transfer/ibb.rb#L19

As with the test case above, a pipe is created with the read end passed to EM#attach. Because the IO is never closed by EventMachine, it's reused by a second file transfer and we get a "broken pipe" error.

The autoclose workaround avoids the error, but it never calls the unbind method on the handler, which is critical to the file transfer processing.

Let me know if you need more info!

Contributor

tmm1 commented Aug 16, 2011

Yea it sounds like autoclose won't really work. The correct fix is to avoid closing the FD on the EM side and let ruby close it explicitly.

@ghost ghost assigned tmm1 Sep 9, 2011

Contributor

tmm1 commented Sep 9, 2011

I am planning a 1.0.0.beta.4 release and would like to have this fixed. Can you try the following patch and confirm it works as expected:

diff --git a/ext/ed.cpp b/ext/ed.cpp
index 10fcbfb..7d6f61d 100644
--- a/ext/ed.cpp
+++ b/ext/ed.cpp
@@ -54,6 +54,7 @@ EventableDescriptor::EventableDescriptor (int sd, EventMachine_t *em):
    bCloseNow (false),
    bCloseAfterWriting (false),
    MySocket (sd),
+   bAttached (false),
    bWatchOnly (false),
    EventCallback (NULL),
    bCallbackUnbind (true),
@@ -179,7 +180,7 @@ void EventableDescriptor::Close()
        MyEventMachine->Deregister (this);

        // Do not close STDIN, STDOUT, STDERR
-       if (MySocket > 2 && !bWatchOnly) {
+       if (MySocket > 2 && !bAttached) {
            shutdown (MySocket, 1);
            close (MySocket);
        }
@@ -458,6 +459,16 @@ void ConnectionDescriptor::SetConnectPending(bool f)


 /**********************************
+ConnectionDescriptor::SetAttached
+***********************************/
+
+void ConnectionDescriptor::SetAttached(bool state)
+{
+   bAttached = state;
+}
+
+
+/**********************************
 ConnectionDescriptor::SetWatchOnly
 ***********************************/

diff --git a/ext/ed.h b/ext/ed.h
index e1b882d..5a81c79 100644
--- a/ext/ed.h
+++ b/ext/ed.h
@@ -103,6 +103,7 @@ class EventableDescriptor: public Bindable_t

    protected:
        int MySocket;
+       bool bAttached;
        bool bWatchOnly;

        EMCallback EventCallback;
@@ -169,6 +170,7 @@ class ConnectionDescriptor: public EventableDescriptor

        void SetNotifyReadable (bool);
        void SetNotifyWritable (bool);
+       void SetAttached (bool);
        void SetWatchOnly (bool);

        bool Pause();
diff --git a/ext/em.cpp b/ext/em.cpp
index dc00741..cd93624 100644
--- a/ext/em.cpp
+++ b/ext/em.cpp
@@ -1356,6 +1356,7 @@ const unsigned long EventMachine_t::AttachFD (int fd, bool watch_mode)
    if (!cd)
        throw std::runtime_error ("no connection allocated");

+   cd->SetAttached(true);
    cd->SetWatchOnly(watch_mode);
    cd->SetConnectPending (false);

diff --git a/lib/eventmachine.rb b/lib/eventmachine.rb
index 6e0fbf3..5a4181e 100644
--- a/lib/eventmachine.rb
+++ b/lib/eventmachine.rb
@@ -736,6 +736,7 @@ module EventMachine
     c = klass.new s, *args

     c.instance_variable_set(:@io, io)
+    c.instance_variable_set(:@watch_mode, watch_mode)
     c.instance_variable_set(:@fd, fd)

     @conns[s] = c
@@ -1396,6 +1397,14 @@ module EventMachine
           else
             c.unbind
           end
+          # If this is an attached (but not watched) connection, close the underlying io object.
+          if c.instance_variable_defined?(:@io) and !c.instance_variable_get(:@watch_mode)
+            io = c.instance_variable_get(:@io)
+            begin
+              io.close
+            rescue Errno::EBADF, IOError
+            end
+          end
         rescue
           @wrapped_exception = $!
           stop
Contributor

tmm1 commented Sep 29, 2011

Can anyone confirm this patch fixes the issue?

dgraham commented Oct 5, 2011

I applied the patch and it fixes the Blather file upload problem I was having. Thanks!

@tmm1 tmm1 closed this in 6d1cdc5 Oct 5, 2011

dv commented Oct 5, 2011

Awesome, thanks tmm1, dgraham!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment