Skip to content
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

Fix for issue #2713 #2714

Closed
wants to merge 1 commit into from
Closed

Conversation

chris-codaio
Copy link

  • Eagerly set the process exitcode in case the write callback isn't executed before the process terminates.
  • Remove the extraneous call to done() at the bottom of the function.
  • Cleanup the draining reduction and value check in done into separate lines for easier reading.

* Eagerly set the process exitcode in case the write callback isn't executed before the process terminates.
* Remove the extraneous call to `done()` at the bottom of the function.
* Cleanup the `draining` reduction and value check in `done` into separate lines for easier reading.
@jsf-clabot
Copy link

jsf-clabot commented Feb 15, 2017

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.188% when pulling ef2b5e5 on chrisleck:issue/2713 into a2fc76c on mochajs:master.

@Munter
Copy link
Contributor

Munter commented Feb 22, 2017

Is is possible to consistently recreate the issue from #2713 ? If it is, a test case to guard against regresssions would be helpful

@Munter Munter added type: bug a defect, confirmed by a maintainer pr-needs-work semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Feb 22, 2017
@chris-codaio
Copy link
Author

I wasn't able to create a standalone test case. Seems timing dependent in node.

@chris-codaio
Copy link
Author

Ping for action?

@chris-codaio
Copy link
Author

Can we get this PR approved or follow up with review comments?

@chris-codaio
Copy link
Author

Ping-a-ling-a-ping-ping

@chris-codaio
Copy link
Author

@Munter Sorry for the direct ping. Mind taking another look?

@Munter
Copy link
Contributor

Munter commented Apr 12, 2017

I understand the value of eagerly setting the exitCode with process.exitCode = .... That part seems good to me.

Without tests I'm not sure what the bigger consequences are of removing the done() call at the end

@chris-codaio
Copy link
Author

@Munter What's your concern about the done() call at the end? The existing code is pretty clearly unbalanced - each stream instance adds one to draining and each callback removes one. The done() call at the end causes draining to reach 0 before the second stream has called done(), thereby defeating the purpose of this ref-counting mechanism.

An alternative to my approach for making the system balanced is to initialize draining to 1 such that the call to done() at the end of the method is the third callback needed to get draining down to 0. That may be even safer than my approach and I'm happy to make that change if you'd like. Please advise.

@chris-codaio
Copy link
Author

@Munter waiting for your response.

@chris-codaio
Copy link
Author

@Munter ping for response.

@ScottFreeCode
Copy link
Contributor

Why not just address the setting of process.exitCode and the correctness or incorrectness of the draining logic (involving the done() call) in separate issues/PRs?

(Actually, we should double-check whether the draining logic's done calls have come up before -- we might already have an issue about investigating it. But in any case, there's benefit to setting process.exitCode once we know how we're going to exit even if we don't touch the draining logic, so...)

@chris-codaio
Copy link
Author

@ScottFreeCode That sounds reasonable. I'll send out a new PR shortly.

chris-codaio added a commit to chris-codaio/mocha that referenced this pull request May 23, 2017
Simpler version of PR mochajs#2714 which only eagerly sets the exitcode without playing with the draining semantics
dasilvacontin pushed a commit that referenced this pull request May 24, 2017
To workaround process being terminated earlier than expected.

Simpler version of PR #2714 which only eagerly sets the exitcode without playing with the draining semantics.
@dasilvacontin
Copy link
Contributor

whether the draining logic's done calls have come up before

@ScottFreeCode can you elaborate?

The logic rewrite looks good to me.

Regarding that extraneous done call, take a look at the old condition:

if (!(draining--)) {

The condition will be true when the value of draining is 0 before hitting the if. draining-- returns then current value of the variable and then it decrements the variable.

So the logic was correct. But laid out in a very confusing way.

@ScottFreeCode
Copy link
Contributor

Thanks, that's what I remember discussing (or at least looking at) before... The parentheses make it particularly confusing because normally they're used to force things to happen in a certain order, but in this case we've got a rare instance of C/C++-style variable-value separation that leaked into JS and even the parentheses don't affect that. (I guess they ensure that the decrement is applied to the variable and not to the result of the negation?)

So it's equivalent to:

if (!drain) {
  ...exit...
}
drain -= 1

...and not to:

drain -= 1
if (!drain) {
  ...exit...
}

And so we don't need to change it for correctness... But we really ought to change it for clarity.

In fact, I would love to see an ESLint rule against postincrement/postdecrement operators. (Wouldn't even say no to such a rule against preincrement/predecrement as well, considering command-query separation ought to be respected if you're going to be using mutable variables in the first place...)

@boneskull
Copy link
Contributor

This was fixed via #2820

@boneskull boneskull closed this Apr 12, 2018
sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
To workaround process being terminated earlier than expected.

Simpler version of PR mochajs#2714 which only eagerly sets the exitcode without playing with the draining semantics.
@JoshuaKGoldberg JoshuaKGoldberg added status: waiting for author waiting on response from OP - more information needed and removed status: pr needs work labels Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" status: waiting for author waiting on response from OP - more information needed type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants