Repository: minrk/wurlitzer

Issue:
Wurlitzer hangs when the GIL is taken by C code
Hello,

I had to debug an issue in a library where I use Wurlitzer. The script I was willing to monitor was using http://caffe.berkeleyvision.org/ which involves some C code and a Python API. This C code output a lot of outputs directly from the C code and don't release the GIL before running the C code.

The results was that the process froze as the main thread was trying to a full pipe and the wurlitzer consuming threads were blocked waiting for the GIL.

I've attached a redacted thread dump from gdb (all the Python threads are waiting for the GIL, I can send the non-redacted thread dump if needed): 
[thread-dumps.log](https://github.com/minrk/wurlitzer/files/2213240/thread-dumps.log)

I've reworked our output monitoring code to reproduce wurlitzer architecture but using multiprocessing.Process instead of threads. This way the consumers process have their own GIL and can run even if the main process is running some C. Moreover I had to monkeypatch `sys.std{out,err}` to force flushing in order to have the last lines of `std{out,err}` before the end of the script.

I also tried to improve the buffer size by following the inspiration from https://github.com/wal-e/wal-e/blob/cdfaa92698ba19bbbff23ab2421fb377f9affb60/wal_e/pipebuf.py#L56. It did worked, but the stdout / stderr was captured after the C code finished, and in my case it could takes hours. So that was not a solution for me.

The changes are pretty big and I'm not sure if they have their place in wurlitzer, what do you think? Anyway I wanted to let you know the potential issue so you can document it or apply wal-e solution to increase pipe buffer size.

Discussion:

Patch:
diff --git a/README.md b/README.md
index 9641b76..e365664 100644
--- a/README.md
+++ b/README.md
@@ -59,13 +59,22 @@ with pipes(logger, stderr=STDOUT):
     call_some_c_function()
 ```
 
+Forward C-level output to a file (avoids GIL issues with a background thread, new in 3.1):
+
+```python
+from wurlitzer import pipes, STDOUT
+
+with open("log.txt", "ab") as f, pipes(f, stderr=STDOUT):
+    blocking_gil_holding_function()
+```
+
 Or even simpler, enable it as an IPython extension:
 
 ```
 %load_ext wurlitzer
 ```
 
-To forward all C-level output to IPython during execution.
+To forward all C-level output to IPython (e.g. Jupyter cell output) during execution.
 
 ## Acknowledgments
 
diff --git a/wurlitzer.py b/wurlitzer.py
index 5626f98..67f61d4 100644
--- a/wurlitzer.py
+++ b/wurlitzer.py
@@ -210,6 +210,17 @@ class Wurlitzer:
         save_fd = os.dup(real_fd)
         self._save_fds[name] = save_fd
 
+        try:
+            capture_fd = getattr(self, "_" + name).fileno()
+        except Exception:
+            pass
+        else:
+            # if it has a fileno(),
+            # dup directly to capture file,
+            # no pipes needed
+            dup2(capture_fd, real_fd)
+            return None
+
         pipe_out, pipe_in = os.pipe()
         # set max pipe buffer size (linux only)
         if self._bufsize:
@@ -272,19 +283,32 @@ class Wurlitzer:
         self._flush()
         # setup handle
         self._setup_handle()
-        self._control_r, self._control_w = os.pipe()
 
         # create pipe for stdout
-        pipes = [self._control_r]
-        names = {self._control_r: 'control'}
+        pipes = []
+        names = {}
         if self._stdout:
             pipe = self._setup_pipe('stdout')
-            pipes.append(pipe)
-            names[pipe] = 'stdout'
+            if pipe:
+                pipes.append(pipe)
+                names[pipe] = 'stdout'
         if self._stderr:
             pipe = self._setup_pipe('stderr')
-            pipes.append(pipe)
-            names[pipe] = 'stderr'
+            if pipe:
+                pipes.append(pipe)
+                names[pipe] = 'stderr'
+
+        if not pipes:
+            # no pipes to handle (e.g. direct FD capture)
+            # so no forwarder thread needed
+            self.thread = None
+            return self.handle
+
+        # setup forwarder thread
+
+        self._control_r, self._control_w = os.pipe()
+        pipes.append(self._control_r)
+        names[self._control_r] = "control"
 
         # flush pipes in a background thread to avoid blocking
         # the reader thread when the buffer is full
@@ -366,11 +390,11 @@ class Wurlitzer:
     def __exit__(self, exc_type, exc_value, traceback):
         # flush before exiting
         self._flush()
-
-        # signal output is complete on control pipe
-        os.write(self._control_w, b'\1')
-        self.thread.join()
-        os.close(self._control_w)
+        if self.thread:
+            # signal output is complete on control pipe
+            os.write(self._control_w, b'\1')
+            self.thread.join()
+            os.close(self._control_w)
 
         # restore original state
         for name, real_fd in self._real_fds.items():


Test patch:
diff --git a/test.py b/test.py
index 5a0b274..0a604c7 100644
--- a/test.py
+++ b/test.py
@@ -206,3 +206,39 @@ def test_log_pipes(caplog):
         # check 'stream' extra
         assert record.stream
         assert record.name == "wurlitzer." + record.stream
+
+
+def test_two_file_pipes(tmpdir):
+
+    test_stdout = tmpdir / "stdout.txt"
+    test_stderr = tmpdir / "stderr.txt"
+
+    with test_stdout.open("ab") as stdout_f, test_stderr.open("ab") as stderr_f:
+        w = Wurlitzer(stdout_f, stderr_f)
+        with w:
+            assert w.thread is None
+            printf("some stdout")
+            printf_err("some stderr")
+
+    with test_stdout.open() as f:
+        assert f.read() == "some stdout\n"
+    with test_stderr.open() as f:
+        assert f.read() == "some stderr\n"
+
+
+def test_one_file_pipe(tmpdir):
+
+    test_stdout = tmpdir / "stdout.txt"
+
+    with test_stdout.open("ab") as stdout_f:
+        stderr = io.StringIO()
+        w = Wurlitzer(stdout_f, stderr)
+        with w as (stdout, stderr):
+            assert w.thread is not None
+            printf("some stdout")
+            printf_err("some stderr")
+        assert not w.thread.is_alive()
+
+    with test_stdout.open() as f:
+        assert f.read() == "some stdout\n"
+    assert stderr.getvalue() == "some stderr\n"