search

jfischoff / tmp-postgres

Create temporary postgres instances
BSD 3-Clause "New" or "Revised" License
53 stars 17 forks source link

Wait for tee'd process stdout/stderr to hit EOF #253

Open mitchellwrosen opened 4 years ago

mitchellwrosen commented 4 years ago

Hi Jonathan,

I looked into debugging this start error that I was seeing (note the empty stdout/stderr):

InitDbFailed {startErrorStdOut = "", startErrorStdErr = "", startErrorExitCode = ExitFailure 1}

And with this patch, it instead looks like

InitDbFailed {startErrorStdOut = "", startErrorStdErr = "initdb: unrecognized option `--no-sync'Try \"initdb --help\" for more information.", startErrorExitCode = ExitFailure 1}

I'll annotate the PR with more info about what's going on

jfischoff commented 4 years ago

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.

mitchellwrosen commented 4 years ago

Will do

mitchellwrosen commented 4 years ago

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? :)

jfischoff commented 4 years ago

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.

mitchellwrosen commented 4 years ago

Hm. Live-lock 👀

jfischoff commented 4 years ago

Hmm it looks like your fix is not passing your test

mitchellwrosen commented 4 years ago

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?

jfischoff commented 4 years ago

Just saw your new changes. Going to review them soon.

Want me to restate what the issue was, and why this patch fixes it?

Sure