-
Notifications
You must be signed in to change notification settings - Fork 29k
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
timers: fix subsequent enroll calls not working #19936
Conversation
A bug was introduced in nodejs#17704 which meant that subsequent calls to enroll would unset the new _idleTimeout and the enrolled object could never again function as a timer.
4423534
to
7ee052e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Point 2 regarding throwing might push this into semver-major territory. |
@jasnell It's marked as dont-land on all the other versions for now. The 1st change is fixing a semver-major commit that hasn't landed in any releases yet. For the second change, I'm fine if we want to have it be semver-major. Or we could also see how it does in 10.x and backport if there's a reason to do so. (It does bring us in line with the browsers and the spec.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok uh I really think these should be in separate PRs:
PR 1 (or separate? whichever)
- Commit 1, fixes an obvious bug of mine which I'm really surprised there wasn't a test for.
- The part that makes the destroy hook fire should probably be fine and seem like a good idea... Maybe I'm for getting something though, since I'm pretty sure me and Trevor had it set like that for some reason.
PR 2
- The browser compat part seems like a breaking change to me. Seems safer to separate it out.
- As in, technically it's a breaking change - but I think you might need to be doing pretty bad already to actually get caught by it.
}; | ||
|
||
timers.enroll(enrollObj, 1); | ||
timers.enroll(enrollObj, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More reliable: put the setTimeout
before this and then just re-enroll with 1
. The list order should guarantee the call order is as we expect without depending on libuv behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but I want to indirectly test that it's getting the new value rather than just keeping the old one without depending on checking _idleTimeout
. That could be another possible bug that someone could introduce.
One of them fixes a breaking change and the other one, I suppose, is a breaking change (although I would call it a bug fix). Feels kind of unnecessary to open another PR now... |
ping @nodejs/tsc since this would ideally make it into 10.x |
I definitely think this should make it to v10 as semver-major, given a clean CITGM run |
Think the one above was ok but I'm not 100% certain what's expected and what's not these days, given my absence for the past month and a bit. The CI was fine too. |
@apapirovski from what I can tell both were fine, it's just common phrasing to indicate that I'm +1 on landing and that that depends on the CI and citgm being green (which they currently are). As in - if we make changes here (which I don't believe we need to at the moment tbh but others might disagree) we should run CITGM again. |
Haha we got our wires crossed. I think I got what you were saying but was trying to express I'm not 100% certain if the CITGM is actually fine or not since I'm not sure what the baseline is these days. 😆 (Indirectly prodding others to verify it for me...) |
Not separating out breaking changes makes backporting significantly more difficult. |
Yeah but there's nothing to back-port here... one is a fix for a semver-major PR (which is landing in 10.x) and the other is semver-major itself. I would understand if they were going into different release lines but right now there's no chance of that happening. |
Oh heh. True. |
Re-running the two failed CI subtasks: https://ci.nodejs.org/job/node-test-commit-linux-containered/3658/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Needs one more TSC approval to land, I think.
Just wondering - Why? |
Breaking changes require sign-off from two TSC members.
(Also: If that text is not sufficiently clear, then that's my fault and let's fix that. I'm 100% certain that the "multiple TSC members" is meant as "approval by at least 2 TSC members is required before landing" because I'm the one who came up with the original language. #7955) |
I'd like to think about the breaking change here a bit more still - we already don't follow browser behavior in several places and we aren't obligated to. Our platform is quite different from the browser for long running tasks and I'd like to consider the implications in people's applications a bit more. |
@Fishrock123 If you want to delay landing this to give more time to think about it, please put a red-X "Request Changes" on it. Otherwise, someone using |
(Also: It might be helpful if you could give a ballpark for how much longer you'd like to think. Days? Weeks? Something else?) |
I don't think the current behaviour is intuitive. There's no indication that the timer won't get rescheduled just because the callback threw an error. In fact, it's mostly an implementation detail. If someone is swallowing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm convinced our current behavior is better for Node.js.
However, I've not seen any good reasoning why we should adopt the browser way of handling errors in this regard, apart from "to increase compatibility". Can you please make a case for this change apart from "to increase compatibility"?
I'd prefer for the enroll()
bug to be on a different PR.
Keep executing it can also cause memory leaks, and sudden behavior changes and bugs exposed in not-well-tested path in code. This is a very edge case: basically we are saying that there is a programmer error (goes to uncaughtException) and the user did not crash their application. This story looks very much like a memory leak to me. |
I fundamentally disagree that compatibility with the spec and browsers is a bad thing. Anyway, I'll back out the setInterval rescheduling change from this PR and open a separate one for it. |
You got me wrong. I think that increasing compatibility is a good thing but I’m not convinced that potentially adding memory leaks to our users code is worth it. |
7ee052e
to
e838531
Compare
@mcollina I backed out the change and left the bug fix. Do you mind rescinding the requested changes? Opening another two PRs with two possible solutions to the interval issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
CI: https://ci.nodejs.org/job/node-test-pull-request/14277/ (one commit was taken out, let's make sure it wasn't crucial to tests passing) |
CI failure is unrelated. |
Seems like the test is flaky: https://ci.nodejs.org/job/node-test-binary-arm/434/RUN_SUBSET=1,label=pi1-docker/console
|
Didn't want to check New CI: https://ci.nodejs.org/job/node-test-pull-request/14323/ |
A bug was introduced in #17704 which meant that subsequent calls to enroll would unset the new _idleTimeout and the enrolled object could never again function as a timer. PR-URL: #19936 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
A bug was introduced in #17704 which meant that subsequent calls to enroll would unset the new _idleTimeout and the enrolled object could never again function as a timer. PR-URL: #19936 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``
This PR resolves one recently introduced bug and one long-standing incompatibility with browser behaviour:
A bug was introduced in a recent
semver-major
PR which makes it impossible toenroll
an alreadyenrolled
object. This resolves the issue by keeping the validation function at the top but moving the assignment of the potentially modifiedmsecs
below.Node.js currently doesn't match browser behaviour when it comes to intervals that throw during their execution. In all browsers, the interval continues running but in Node.js it will never be rescheduled after a throw nor will the destroy async hook for that interval fire.Edit: The second change has been backed out now.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes