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

fs: do not emit 'stop' watch event synchronously #8524

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@claudiorodriguez
Member

claudiorodriguez commented Sep 13, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Emits 'stop' event for fs.watchFile on process.nextTick
to fix 'maximum call stack size exceeded' error when
stop is called synchronously after listener is attached.

Fixes: #8421

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Sep 13, 2016

LGTM pending CI.

@mscdex

This comment has been minimized.

Contributor

mscdex commented Sep 13, 2016

@imyller

This comment has been minimized.

Member

imyller commented Sep 14, 2016

@claudiorodriguez Just wondering if setImmediate instead of nextTick should be used here?

https://github.com/nodejs/node/blob/master/doc/topics/the-event-loop-timers-and-nexttick.md#processnexttick-vs-setimmediate states that:

We recommend developers use setImmediate() in all cases because it's easier to reason about (and it leads to code that's compatible with a wider variety of environments, like browser JS.)

setImmediate is bit more asynchronous since it fires on the following iteration or 'tick' of the event loop; meaning that stop event would be emitted there.

I noted that nextTick is the preferred solution in core lib/. Maybe for a good reason.

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Sep 15, 2016

@imyller just tried that change locally, it passes all tests successfully. I'd be inclined to make the change, it makes sense, although maybe someone from @nodejs/collaborators can chime in, if process.nextTick is preferred for a particular reason.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 15, 2016

process.nextTick() is lower latency, it's dispatched as quickly as possible. setImmediate() postpones the callback until the next loop tick.

(process.nextTick() was an apt name when it was introduced but not anymore.)

@yorkie

This comment has been minimized.

Member

yorkie commented Sep 15, 2016

@claudiorodriguez 2 suggestions from me:

  • the test file should be renamed to test-fs-watch-stop-sync.js.
  • we should also add a test file test-fs-watch-stop-async.js to make sure that the trigger calls emit() in next ticks.
@jasnell

While I'm good with this change in general, I'm wondering if this needs to be semver-major due to the change in timing? I don't believe it does but want to be careful

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Sep 16, 2016

@yorkie good idea, I'll do that this weekend

@jasnell I'm trying to think of a way in which userland code listening to the emitter would break, but can't think of any - seeing as how the contract remains the same. Can you think of any user code that would behave differently?

@claudiorodriguez claudiorodriguez force-pushed the claudiorodriguez:fs-stop-nexttick branch Sep 19, 2016

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Sep 19, 2016

@yorkie added the test, PTAL

New CI: https://ci.nodejs.org/job/node-test-pull-request/4123/
(although CI seems to have been failing for a while)

lib/fs.js Outdated
self.emit('stop');
process.nextTick(function() {
self.emit('stop');
});

This comment has been minimized.

@mcollina

mcollina Sep 19, 2016

Member

Can we avoid allocating a new closure here? When calling stop() on a massive tree of objects, we will be allocating a lot of closures for nothing.

this._handle.onstop = function() {
  process.nextTick(emitStop, self)
}

// to be put a root level
function emitStop(self) {
  self.emit('stop');
}

This comment has been minimized.

@claudiorodriguez
fs: do not emit 'stop' watch event synchronously
Emits 'stop' event for fs.watchFile on process.nextTick
to fix 'maximum call stack size exceeded' error when
`stop` is called synchronously after listener is attached.

Fixes: #8421

@claudiorodriguez claudiorodriguez force-pushed the claudiorodriguez:fs-stop-nexttick branch to e4028ac Sep 19, 2016

@imyller

LGTM

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 20, 2016

@claudiorodriguez ... no, I can't think of anything either. As I said, this should be perfectly fine.

@jasnell

LGTM

@Trott Trott force-pushed the nodejs:master branch to c5ce7f4 Sep 21, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 22, 2016

@yorkie

yorkie approved these changes Sep 23, 2016

LGTM :)

@imyller imyller self-assigned this Sep 23, 2016

@imyller

This comment has been minimized.

Member

imyller commented Sep 23, 2016

Landing:

  • Five LGTMs
  • No objections
  • CI test passed (only the known CI failures)

/ping @jasnell you were thinking about adding semver-major label for this. Still relevant?

imyller added a commit to imyller/node that referenced this pull request Sep 23, 2016

fs: do not emit 'stop' watch event synchronously
Emits 'stop' event for fs.watchFile on process.nextTick
to fix 'maximum call stack size exceeded' error when
`stop` is called synchronously after listener is attached.

PR-URL: nodejs#8524
Fixes: nodejs#8421
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
@imyller

This comment has been minimized.

Member

imyller commented Sep 23, 2016

landed in 1b97774

@imyller

This comment has been minimized.

Member

imyller commented Sep 23, 2016

To be safe, I've added dont-land- labels until semver level is discussed and resolved.

jasnell added a commit that referenced this pull request Sep 29, 2016

fs: do not emit 'stop' watch event synchronously
Emits 'stop' event for fs.watchFile on process.nextTick
to fix 'maximum call stack size exceeded' error when
`stop` is called synchronously after listener is attached.

PR-URL: #8524
Fixes: #8421
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
@addaleax

This comment has been minimized.

Member

addaleax commented Oct 1, 2016

Hm, bump? Does anybody feel this should be semver-major? If not, I’d like to remove the labels.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 1, 2016

Changes event timing, sounds major to me

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment