fix on_recv called when None after close

[graingert]

Fixes #82

[graingert]

@mmerickel can you approve my workflow build please?

[graingert]

thanks looks like there's an fd use after close bug:

>               warnings.warn(pytest.PytestUnhandledThreadExceptionWarning(msg))
E               pytest.PytestUnhandledThreadExceptionWarning: Exception in thread Thread-1
E               
E               Traceback (most recent call last):
E                 File "/Users/runner/hostedtoolcache/PyPy/3.8.16/x64/lib/pypy3.8/threading.py", line 932, in _bootstrap_inner
E                   self.run()
E                 File "/Users/runner/hostedtoolcache/PyPy/3.8.16/x64/lib/pypy3.8/threading.py", line 870, in run
E                   self._target(*self._args, **self._kwargs)
E                 File "/Users/runner/work/hupper/hupper/.tox/py/lib/pypy3.8/site-packages/hupper/ipc.py", line 167, in _read_loop
E                   packet = self._recv_packet()
E                 File "/Users/runner/work/hupper/hupper/.tox/py/lib/pypy3.8/site-packages/hupper/ipc.py", line 144, in _recv_packet
E                   chunk = os.read(self.r_fd, self._packet_len.size)
E               OSError: [Errno 9] Bad file descriptor

[graingert]

@mmerickel can I get another run approved please?

[graingert]

Apparently os.read(-1) hangs on windows?

looks like Connection.close() hangs on windows, this happens on main too if Connection is tested in isolation

[graingert]

this always hangs forever on windows:

import os
import sys
import threading

def consume(ident, fd):
    while chunk := os.read(fd, 1):
        print(ident, chunk)

def main():
    cr, cw = os.pipe()
    crt = threading.Thread(target=consume, args=("cr", cr))
    crt.start()

    print("close cr", flush=True)
    os.close(cr)
    print("close cw", flush=True)
    os.close(cw)

    

if __name__ == "__main__":
    sys.exit(main())

[graingert]

@mmerickel ok I tested this on linux and windows now it should pass in CI!

[shughes-uk]

This is a constant annoyance I have using hupper, would love to see it fixed!

[mmerickel]

@graingert I finally got a chance to sit down and think about this and I'm wondering if we can simplify this to a one-line fix? What do you think?

diff --git a/src/hupper/ipc.py b/src/hupper/ipc.py
index cec1a85..11fdba3 100644
--- a/src/hupper/ipc.py
+++ b/src/hupper/ipc.py
@@ -135,9 +135,9 @@ class Connection(object):
         self.reader_thread.start()
 
     def close(self):
+        self.on_recv = lambda _: None
         close_fd(self.r_fd)
         close_fd(self.w_fd)
-        self.on_recv = None
 
     def _recv_packet(self):
         buf = io.BytesIO()

[graingert]

Doesn't fix the use after close

[graingert]

@mmerickel I've pushed that change

[mmerickel]

How does it not fix the use-after-close? The exception you posted in the original ticket is showing that things were properly caught (by the EOFError?) but the on_recv was None at the time instead of something that could be invoked.

[mmerickel]

ok I see - it's not in your original stacktrace but I agree that invoking read on a closed fd will return EBADF which is not currently handled so your fix improves that.

[mmerickel]

Please modify the PR to set self.on_recv before closing the fds to avoid a race there. I think there is no real point in setting the fds to -1 either but if you want to keep that it's ok. Thanks!

The following to me is a proper fix:

diff --git a/src/hupper/ipc.py b/src/hupper/ipc.py
index cec1a85..bbc567a 100644
--- a/src/hupper/ipc.py
+++ b/src/hupper/ipc.py
@@ -1,3 +1,4 @@
+import errno
 import io
 import os
 import pickle
@@ -135,9 +136,9 @@ class Connection(object):
         self.reader_thread.start()
 
     def close(self):
+        self.on_recv = lambda _: None
         close_fd(self.r_fd)
         close_fd(self.w_fd)
-        self.on_recv = None
 
     def _recv_packet(self):
         buf = io.BytesIO()
@@ -166,6 +167,9 @@ class Connection(object):
                 self.on_recv(packet)
         except EOFError:
             pass
+        except OSError as e:
+            if e.errno != errno.EBADF:
+                raise
         self.on_recv(None)
 
     def _write_packet(self, data):

[mmerickel]

@graingert I haven't had time to debug this but something is clearly wrong with the new test you added on windows. Obviously one run passes, but all the others fail. I tried to re-run the failed jobs and they failed again in the same way with a deadlock in those tests.

[mmerickel]

Whelp, 4 hours later after installing a windows VM and banging my head for a while I found the issue. Basically you cannot invoke os.close(r_fd) if the other thread is blocking on os.read(r_fd). Thanks windows. It appears to work reliably if you close the write_fd before the read_fd.