Fix: Always close the reporter before exiting #4283
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpojer
approved these changes
Aug 29, 2017
BYK
approved these changes
Aug 29, 2017
|
Hm Flow seems to recognize Re: tests, if you think it's better I'll add one. |
|
If it is not too much trouble, a test would be great. If not, well... 🤷♂️ |
BYK
approved these changes
Aug 30, 2017
BYK
approved these changes
Sep 4, 2017
| @@ -152,6 +152,8 @@ export default class BaseReporter { | |||
| this.peakMemoryInterval = setInterval(() => { | |||
| this.checkPeakMemory(); | |||
| }, 1000); | |||
| // $FlowFixMe: Node's setInterval returns a Timeout, not a Number | |||
| this.peakMemoryInterval.unref(); | |||
There was a problem hiding this comment.
I'm 👍 with this change but curious why clearInterval(this.peakMemoryInterval); in close() below isn't enough.
src/cli/index.js
Outdated
| @@ -349,10 +349,10 @@ export function main({ | |||
| return errorReportLoc; | |||
| } | |||
|
|
|||
| const exit = exitCode => { | |||
| function exit(exitCode) { | |||
There was a problem hiding this comment.
Why convert to "normal" functions from arrow functions?
There was a problem hiding this comment.
Because otherwise exit is undefined at lines 178 & 195, since the assignment hasn't been executed yet.
There was a problem hiding this comment.
Ah, let's move this above then? Or add a comment if moving up is not possible?
arcanis
force-pushed
the
interval-peak-unref
branch
from
5276535
to
58f5d09
Compare
arcanis
force-pushed
the
interval-peak-unref
branch
from
58f5d09
to
31925a0
Compare
|
I don't think it fixes #4276, although this is a welcome PR. |
BYK
pushed a commit
that referenced
this pull request
Sep 13, 2017
**Summary** A combination of changes have caused `yarn upgrade-interactive` to exit with a promise rejection. In short, I believe it has always been a problem, but #3995 exposed it to the prompt. The child rejection inside of [upgrade-interactive](https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/upgrade-interactive.js#L152) is fine, as it was handled by the `Promise.race` condition; however, rejecting at the parent level inside of [console-reporter](https://github.com/yarnpkg/yarn/blob/master/src/reporters/console/console-reporter.js#L458) causes[ loud-rejection](https://github.com/sindresorhus/loud-rejection) to handle this. I believe @arcanis 's PR #4283 is what would allow us not to hook into `SIGINT` inside of the console reporter and allow the reporter to cleanly close itself. **Test Plan** Will work on some scenarios! This PR needs some more verification on my end ... @BYK @torifat @arcanis please jump in and provide any feedback you think could be helpful! Opened early for visibility :)
joaolucasl
pushed a commit
to joaolucasl/yarn
that referenced
this pull request
Oct 27, 2017
* Prevents the memory controls from hanging the process * Update base-reporter.js * Adds a test
joaolucasl
pushed a commit
to joaolucasl/yarn
that referenced
this pull request
Oct 27, 2017
**Summary** A combination of changes have caused `yarn upgrade-interactive` to exit with a promise rejection. In short, I believe it has always been a problem, but yarnpkg#3995 exposed it to the prompt. The child rejection inside of [upgrade-interactive](https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/upgrade-interactive.js#L152) is fine, as it was handled by the `Promise.race` condition; however, rejecting at the parent level inside of [console-reporter](https://github.com/yarnpkg/yarn/blob/master/src/reporters/console/console-reporter.js#L458) causes[ loud-rejection](https://github.com/sindresorhus/loud-rejection) to handle this. I believe @arcanis 's PR yarnpkg#4283 is what would allow us not to hook into `SIGINT` inside of the console reporter and allow the reporter to cleanly close itself. **Test Plan** Will work on some scenarios! This PR needs some more verification on my end ... @BYK @torifat @arcanis please jump in and provide any feedback you think could be helpful! Opened early for visibility :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix #4276
In some particular cases, the reporters are not correctly closed before Yarn exits. It makes the process hang, because the reporters are expected to periodically check for the RAM usage. This patch does two things:
unref, so that it will never prevent the process from exiting.Test plan
I don't think tests are required for this fix, since the reproduction steps are fairly narrow (the bug only occurred when using Yarn in an invalid way) and won't allow us to catch any other bug than this one.