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

lib: fixme defer error to next tick #4670

Closed

Conversation

@tflanagan
Copy link
Contributor

@tflanagan tflanagan commented Jan 13, 2016

Fixes FIXMEs in lib/internal/child_process.js ref's #4642

...
      process.nextTick(callback, ex);
    } else {
      this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
    }
    return false;
...
        process.nextTick(callback, ex);
      } else {
        this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
      }
    }
@jasnell
Copy link
Member

@jasnell jasnell commented Jan 13, 2016

Marking this as a possible semver-minor due to the change in the timing. @bnoordhuis .. thoughts on this one?

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 13, 2016

Wouldn't a change in timing be a semver-major change?

@jasnell
Copy link
Member

@jasnell jasnell commented Jan 14, 2016

Possibly yes.

@tflanagan tflanagan force-pushed the tflanagan:internal-cp-fixme-defer-nt branch to 2c41576 Jan 22, 2016
@tflanagan
Copy link
Contributor Author

@tflanagan tflanagan commented Jan 22, 2016

I've updated this PR to match master

@jasnell
Copy link
Member

@jasnell jasnell commented Jan 23, 2016

@bnoordhuis ... thoughts?

@tflanagan
Copy link
Contributor Author

@tflanagan tflanagan commented Feb 17, 2016

/poke @bnoordhuis

@thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Feb 18, 2016

Related work: #5251

@jasnell
Copy link
Member

@jasnell jasnell commented Mar 22, 2016

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Mar 22, 2016

LGTM if tests pass. I'd make it semver-major just to be safe. CI: https://ci.nodejs.org/job/node-test-pull-request/2027/

@jasnell jasnell added semver-major and removed semver-minor labels Mar 22, 2016
@estliberitas estliberitas force-pushed the nodejs:master branch 2 times, most recently to c7066fb Apr 26, 2016
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Jun 17, 2016

should we add this to the v7 milestone?

@imyller
Copy link
Member

@imyller imyller commented Sep 15, 2016

FIXME states that emitting error should be deferred to next tick. setImmediate does this.

process.nextTick and setImmediate have confusing names, since setImmediate will actually defer emitting the error to next tick and process.nextTick will emit error within current tick cycle.

Thus nextTick has lower latency, but does it matter in this case?

@Trott
Copy link
Member

@Trott Trott commented Sep 16, 2016

LGTM. Since it's semver-major (out of caution, not because this is actually expected to cause huge problems), I'm going to ping @nodejs/ctc to see if anyone else wants to LGTM or wait wait wait wat no no no this.

Also, since it's been dormant a while, might be good to run one last CI. Also a CITGM because: semver-major.

CI: https://ci.nodejs.org/job/node-test-pull-request/4076/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/387/

@mscdex
Copy link
Contributor

@mscdex mscdex commented Sep 16, 2016

IMHO I don't think it has to be until the next actual tick, I think the original idea was to allow the user to have time to set up an error event handler before it's emitted. process.nextTick() accomplishes this without further unnecessary delay. It's also what is used throughout the code base whenever similar situations arise.

@imyller
Copy link
Member

@imyller imyller commented Sep 16, 2016

Agree with @mscdex

LGTM pending new CI and ctc comments

Copy link
Contributor

@cjihrig cjihrig left a comment

LGTM

@santigimeno
Copy link
Member

@santigimeno santigimeno commented Sep 19, 2016

LGTM

@evanlucas
Copy link
Member

@evanlucas evanlucas commented Sep 19, 2016

@Trott Trott force-pushed the nodejs:master branch to c5ce7f4 Sep 21, 2016
@rvagg rvagg force-pushed the nodejs:master branch 2 times, most recently to 83c7a88 Oct 18, 2016
@MylesBorins MylesBorins force-pushed the nodejs:master branch to 54fef67 Feb 1, 2017
@jasnell
Copy link
Member

@jasnell jasnell commented Feb 28, 2017

ping @nodejs/ctc ... did we want to land this?

@jasnell jasnell added the stalled label Mar 1, 2017
@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017
@fhinkel
Copy link
Member

@fhinkel fhinkel commented May 23, 2017

There's two approvals and no rejection, so I'd say...yes?

@gibfahn
Copy link
Member

@gibfahn gibfahn commented May 23, 2017

3 ctc approvals (@bnoordhuis @Trott @cjihrig ) and 2 other collaborator ones ( @imyller @santigimeno) so it should be good to land, probably needs another CI and CitGM run though.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jun 18, 2017

Seems like this is not stalled and it only has to land?

@Trott Trott removed the stalled label Aug 10, 2017
Trott added a commit to Trott/io.js that referenced this pull request Aug 13, 2017
PR-URL: nodejs#4670
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@Trott
Copy link
Member

@Trott Trott commented Aug 13, 2017

Landed in f2b01cb.

Triageathon 2017!

@Trott Trott closed this Aug 13, 2017
@SimenB SimenB mentioned this pull request Oct 19, 2017
6 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.