Remove exit module #1384
Remove exit module #1384
Conversation
|
Reverts #1296 |
|
Previous bug/misfeature discovered in exit: #1364 |
smfreegard
force-pushed
the
remove_exit
branch
2 times, most recently
from
9c6f6b4
to
d6c77ac
Compare
|
Is there a better fix we should look at? |
|
@baudehlo How do you mean? As you pointed out, the bug that exit() supposedly worked around was fixed in Node anyway. |
Well, #1296 did address a real issue that also resulted in difficult-to-track-down errors. The fact that it also introduced a couple is a cost of accepting PRs without proper test coverage. There's two ways to go about this:
|
|
The problem with #2 is I suspect this is platform and node version dependant. I've never seen it (but then I'm still stuck on v0.10 everywhere due to shitty compilers).
|
|
Being as we don't have any integration tests at the moment, exposing either the original issue or the nasty crashes that I've had would is well beyond my skill to do so (e.g. how the heck within an integration test framework do you spool up Haraka, deliberately cause one of the children an exception, then detect that whilst it's still running, the master is endlessly spawning children which die off immediately), likewise how do you write to stdout but detect that due to async issues it wasn't actually written out before the process exited. As far as I'm concerned we're better off reverting this ASAP as it's currently horribly broken. I'd rather look at the original issue and generate an EACCESS issue and then see if it's fixable by moving some of the code around in haraka.js or server.js as I seriously don't like the exit() module now, it's a horrible hack. |
|
+1 On Thu, Mar 10, 2016 at 6:57 PM, Steve Freegard notifications@github.com
|
|
If we decide to revisit this, it seems one option might be: if (I am a cluster child) process.exit() else library_exit()
|
|
Ok - I've spent some more time on this today. I was able to reproduce #1296 on iojs 1.8.4 - I only get the full output some of the time: The workaround is to pipe to cat (which highlights a bug in logger.dump_logs()): The "missing" output in the latter is because logger.dump_logs only dumps the logs when process.stdout.isTTY is true, which looks very much like a bug to me.
True, but in my case this all blew up in haraka.js, so we'd have to use environment variables to detect if we're parent or child. My concern would be what other bugs lay down that route, I couldn't reliably reproduce the conditions that caused my crashes which is why it took so long to find it. I was able to reliably get all log output without any pipes of workarounds by doing this: And then changing the calls to it like so: I admit I don't really understand why that works See nodejs/node#2972 (comment) If that's an acceptable workaround, then I'd rather do it this way than to use exit(). |
|
+1
|
|
Ok - I've refactored the necessary changes and created a new logger.dump_and_exit(code) method which is used instead. This fixes the issue reported in #1296 and reverts the changes it made. |
Remove the exit module as it's caused me multiple nasty issues.
The latest issue I've had was extremely hard to diagnose (it required strace) which caused the cluster master to be unable to respawn a crashed child process:
It looks like what happened is that exit() somehow manages to close stdin/stdout/stderr in the master process when one of the children hit an unhandled exception and this prevented all future children spawned by master from inheriting them and therefore at start-up when they attempted to write to the console, they immediately hit an EPIPE exception and would then crash out again. Rinse repeat.