Wait for tee'd process stdout/stderr to hit EOF #253
Conversation
![hint-man[bot]](https://avatars.githubusercontent.com/in/20170?s=60&v=4){kind=small}
| @@ -132,7 +132,7 @@ teeHandle orig f = | |||
| modifyIORef outputRef (<>theLine) | |||
| hPutStrLn orig theLine | |||
|
|
|||
| res <- withAsync readerLoop $ \_ -> f writeEnd | |||
| res <- withAsync readerLoop $ f writeEnd | |||
There was a problem hiding this comment.
The callback is now provided a handle to the background thread that's reading readEnd.
Previously, after initdb finished, the callback was closed immediately, which canceled this thread before it could read the pipe until EOF.
| { completeProcessConfigStdErr = newErr | ||
| , completeProcessConfigStdOut = newOut | ||
| } | ||
| for_ [newOut, newErr] hClose -- executeProcess doesn't close these |
There was a problem hiding this comment.
executeProcess doesn't close newErr or newOut because it uses createProcess_ internally, so we explicitly do that now. This causes the background threads to die eventually when they try to hGetLine on a handle that's hit EOF.
There was a problem hiding this comment.
One thing I wasn't totally sure about: we close the write ends here (so the read threads die), but we also close the write ends in the cleanup function of the bracketed createPipe. So, the handles are closed twice. Things seem to work, though; I guess I'd have expected that to be an IO exception.
There was a problem hiding this comment.
Oh, scratch that. From the docs:
Performing hClose on a handle that has already been closed has no effect; doing so is not an error.
|
I'm happy to merge this PR as is but I think if would be best to add test case that fails without this PR and passes with it. |
|
Will do |
|
Hm. I attempted to add a failing test and follow it up with the commit that fixes it, but the commit that was supposed to fail passed CI. It's racy - the background threads might exhaust the handles before they're killed, which makes the test pass. Maybe I could just reword the commit to not claim the test is "failing", follow up with the original commit, and call it good? :) |
|
Hmm. My 500 foot view tells me there are still some mysteries here to solve. I'll try to take a look just to make sure I understand the problem and the fix. Thanks for adding the test ... I'll see if I can help make it reproducible. |
| @@ -128,6 +128,7 @@ teeHandle orig f = | |||
| outputRef <- newIORef [] | |||
|
|
|||
| let readerLoop = forever $ do | |||
| threadDelay 1000000 | |||
There was a problem hiding this comment.
Yeah; just added this temporarily to show where it's racy
|
Hm. Live-lock 👀 |
|
Hmm it looks like your fix is not passing your test |
|
I'm not sure what the issue was, but I reworked the code a little bit and got the tests passing. Not very reassuring but... I think the logic checks out. Want me to restate what the issue was, and why this patch fixes it? |
|
Just saw your new changes. Going to review them soon.
Sure |
Hi Jonathan,
I looked into debugging this start error that I was seeing (note the empty stdout/stderr):
And with this patch, it instead looks like
I'll annotate the PR with more info about what's going on