FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: fixme defer error to next tick by tflanagan · Pull Request #4670 · nodejs/node · GitHub

/ node Public

lib: fixme defer error to next tick - #4670

Closed
tflanagan wants to merge 1 commit into
nodejs:masterfrom
tflanagan:internal-cp-fixme-defer-nt
ConversationCommits (1)ChecksFiles changed
Closed

lib: fixme defer error to next tick#4670
tflanagan wants to merge 1 commit into
nodejs:masterfrom
tflanagan:internal-cp-fixme-defer-nt

Conversation

tflanagan commented Jan 13, 2016

Copy link
Copy Markdown
Contributor

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.
      }
    }

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Jan 13, 2016
jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 13, 2016

jasnell commented Jan 13, 2016

Copy link
Copy Markdown
Member

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

cjihrig commented Jan 13, 2016

Copy link
Copy Markdown
Contributor

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

jasnell commented Jan 14, 2016

Copy link
Copy Markdown
Member

Possibly yes.

tflanagan force-pushed the internal-cp-fixme-defer-nt branch from fece1f9 to 2c41576 Compare January 22, 2016 19:40

tflanagan commented Jan 22, 2016

Copy link
Copy Markdown
Contributor Author

I've updated this PR to match master

jasnell commented Jan 23, 2016

Copy link
Copy Markdown
Member

@bnoordhuis ... thoughts?

tflanagan commented Feb 17, 2016

Copy link
Copy Markdown
Contributor Author

/poke @bnoordhuis

Trott commented Feb 18, 2016

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/1692/

thefourtheye commented Feb 18, 2016

Copy link
Copy Markdown
Contributor

Related work: #5251

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

@bnoordhuis @tflanagan ... ping

bnoordhuis commented Mar 22, 2016

Copy link
Copy Markdown
Member

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 added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 22, 2016
estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

MylesBorins commented Jun 17, 2016

Copy link
Copy Markdown
Contributor

should we add this to the v7 milestone?

imyller commented Sep 15, 2016

Copy link
Copy Markdown
Member

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 commented Sep 16, 2016

Copy link
Copy Markdown
Member

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 commented Sep 16, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Sep 16, 2016

Copy link
Copy Markdown
Member

Agree with @mscdex

LGTM pending new CI and ctc comments

cjihrig approved these changes Sep 17, 2016
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

santigimeno commented Sep 19, 2016

Copy link
Copy Markdown
Member

LGTM

evanlucas commented Sep 19, 2016

Copy link
Copy Markdown
Contributor

Running CI one more time: https://ci.nodejs.org/job/node-test-pull-request/4126/

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

jasnell commented Feb 28, 2017

Copy link
Copy Markdown
Member

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

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

fhinkel commented May 23, 2017

Copy link
Copy Markdown
Contributor

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

gibfahn commented May 23, 2017

Copy link
Copy Markdown
Member

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 commented Jun 18, 2017

Copy link
Copy Markdown
Member

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

Trott removed the stalled Issues and PRs that are stalled. label Aug 10, 2017

Trott commented Aug 10, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/9587/

Trott pushed a commit to Trott/io.js that referenced this pull request Aug 13, 2017
child_process: defer error to next tick
f2b01cb 
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 commented Aug 13, 2017

Copy link
Copy Markdown
Member

Landed in f2b01cb.

Triageathon 2017!

Trott closed this Aug 13, 2017
SimenB mentioned this pull request Oct 19, 2017
Closed
8 tasks
jasnell mentioned this pull request Oct 30, 2017
Merged
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

santigimeno santigimeno approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL