#48 - Flush writer pipe; don't close it. #49
Conversation
NHDaly
changed the title
Fixes #47759: Flush writer pipe; don't close it. Since closing the pipe can cause race conditions with someone writing to the pipe, it's better to flush it, and then leave closing the pipe endpoint to GC.
This way we achieve all the goals: - We close the writing task from our side, in a way that doesn't impact any other task that has captured the logger. The other task's output will just be silently dropped. - We still allow the Pipe()s to be gracefully closed by GC, once they're no longer referenced.
| # https://github.com/JuliaLang/julia/issues/40626) | ||
| @async nothing | ||
| end | ||
| end |
There was a problem hiding this comment.
😕 okay SO: this does fix the tests, and this is now a working solution.
HOWEVER: It made me realize one other caveat with this approach.
Before, a task could throw an exception if it attempted to log from one thread while another thread is doing @capture_err.
NOW: it's possible for its log message to be SILENTLY DROPPED during that race condition, which is maybe just as bad?
So maybe the real answer is actually to fix JuliaLang/julia#47759 on the logger side, and leave this package alone? :/
It's sad because this fix seemed easier, but maybe less so now after these changes.
| fetch(out_reader) | ||
| s = String(take!(io_buf)) | ||
| # Now close the task to allow the Pipe() to be GC'd | ||
| close(io_buf) |
There was a problem hiding this comment.
I think at this point you need write something to the pipe and flush to make sure the task dies.
This reverts commit 215c69a.
Fixes #48: Flush writer pipe; don't close it.
Since closing the pipe can cause race conditions with someone writing to the pipe, it's better to flush it, and then leave closing the pipe endpoint to GC.