-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes #860: failure to exit on SIGINT race condition #1157
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1157 +/- ##
======================================
Coverage 76.8% 76.8%
======================================
Files 5 5
Lines 470 470
Branches 151 151
======================================
Hits 361 361
Misses 109 109 Continue to review full report at Codecov.
|
@@ -390,8 +390,9 @@ function startDevServer(webpackOptions, options) { | |||
|
|||
['SIGINT', 'SIGTERM'].forEach((sig) => { | |||
process.on(sig, () => { | |||
server.close(); | |||
process.exit(); // eslint-disable-line no-process-exit | |||
server.close(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process.nextTick()
? But not 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-ciniawsky hm? please elaborate :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think it is bananas from my side tbh :), the intend was
server.close()
process.nextTick(() => process.exit())
to ensure server.close()
finishes before process.exit()
is called by pushing it to the next EventLoop 'tick'. But a callback to server.close()
is definitely cleaner and more expressive/logical, so forget what I said 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha no worries. something good to keep in mind though!
What kind of change does this PR introduce?
Properly calls
process.exit
only afterserver.close
has completed. Probable fix for reported edge case whereby the server process doesn't actually exit when SIGINT is sent.Did you add or update the
examples/
?No need
Summary
See above
Does this PR introduce a breaking change?
No
Other information
Added new test to confirm SIGINT detection works properly.