timers: do not expose .unref()._handle._list #8422

Fishrock123 · 2016-09-06T15:52:10Z

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)

timers

Description of change

The list is defunct at this point.
I believe this to be in effect a minor defect/oversight from
3eecdf9

Tangentially effects/related to #8372

Edit: cc @indutny?

YurySolovyov · 2016-09-06T17:02:24Z

lib/timers.js

@@ -481,6 +481,7 @@ Timeout.prototype.unref = function() {
    }

    var handle = reuse(this);
+    delete handle._list;


I thought V8 don't like deleting props prom objects, but it might be ok if it is not perf-sensitive...

Under #8372 this code path is far less likely to be hit in the first place.

I doubt the overhead from a later-than-this-tick unref() is much of a worry.

Why not removing the reference to the TimersList instance in reuse's implementation instead?

Also, #8372 hasn't landed yet, and it's not clear whether it will land. As a result I would think that if deleting the ._list property is more costly than setting it to null (or doing anything else that we know might perform better), we should not do that and instead do what we know performs better.

Why not removing the reference to the TimersList instance in reuse's implementation instead?

Messes with some tests that check things after unenroll(), which also uses reuse().

Setting it to undefined would be faster but I figured it could be confusing that sometimes there would be a _list property.

If that is acceptable we can switch to it though.

Why not removing the reference to the TimersList instance in reuse's implementation instead?

Messes with some tests that check things after unenroll(), which also uses reuse().

Making that change (clearing the handle's reference to the TimersList instance) makes only one test fail on master: test/parallel/test-timers-same-timeout-wrong-list-deleted.js. I think that test could be updated to not rely on that reference being present for unenrolled timers.

Is this what you had in mind by "Messes with some tests"?

@Fishrock123

delete is known to be a heavy operation.

I thought we were also considering setting the property to null or undefined, isn't it still the case?

You mentioned earlier:

Setting it to undefined would be faster but I figured it could be confusing that sometimes there would be a _list property.

If that is acceptable we can switch to it though.

Having a _list property set to null or undefined seems like it would be acceptable.

Should we just use Symbols for _list?

We could, I thought those were exposable somehow too?

I still think we should unreference it in some way though so the list can be GC'd.

Using a Symbol and setting the value to undefined would accomplish both goals without incurring the potential perf hit of the delete, yes? Symbols are exposable but not without extra work.

or, why don't we set it to null. we already do that for some other properties like _handle.

Fishrock123 · 2016-09-06T21:01:16Z

cc also @misterdjules maybe?

misterdjules · 2016-09-06T22:10:25Z

The idea seems reasonable, I added a couple comments regarding the implementation.

misterdjules · 2016-09-12T23:52:17Z

It seems that removing the ._list property would be a breaking change. Setting it to null or undefined could be considered as a breaking change too. As a result it seems this change should be labelled semver-major.

Fishrock123 · 2016-09-13T04:33:42Z

It seems that removing the ._list property would be a breaking change.

Theoretically but I'm willing to bet no one even knew it existed.

misterdjules · 2016-09-13T05:44:35Z

@Fishrock123

It seems that removing the ._list property would be a breaking change.

Theoretically but I'm willing to bet no one even knew it existed.

Can we use https://github.com/ChALkeR/Gzemnid to look into that? I would like to err on the side of caution regardless of the results, unless the benefits of the change far outweigh the risks, however unlikely.

Fishrock123 · 2016-09-13T22:02:44Z

cc @ChALkeR?

I dunno how this would be detected, but I guess you could look for ._list?

ChALkeR · 2016-09-15T18:00:52Z

There is a lot of stuff on \._list[^a-zA-Z0-9_], but the lines I seen don't look timers related.
Also a lot of stuff on \._handle[^a-zA-Z0-9_].

Nothing on handle\._list, but that doesn't guarantee anything.
I will take a closer look soon.

Fishrock123 · 2017-01-05T18:19:56Z

@nodejs/ctc Anyone object to me merging this one as a major?

The _handle._list double-private property is only exposed under a specific condition and is non-functional when it is exposed.

targos · 2017-01-06T09:56:51Z

+1

trevnorris · 2017-01-11T23:13:48Z

@Fishrock123 If it's non-functional when it's exposed then I'd consider this a bug fix. Or at most a minor. Calling this a major seems like far overkill. We're removing a property that's broken right?

Fishrock123 · 2017-01-12T16:16:11Z

@trevnorris The property exists, but it doesn't actually have any relevant functionality since it is circularly attached to a handle which is repurposed for an individual timeout.

Fishrock123 · 2017-01-12T16:16:44Z

@indutny What's the point? This is the only spot where it is exposed to a user.

jasnell · 2017-03-01T02:05:42Z

ping @Fishrock123 ... still making progress on this?

jasnell · 2017-03-22T22:25:13Z

Ping @Fishrock123 ...

Fishrock123 · 2017-03-22T23:00:29Z

@indutny Is there a large benefit here to using Symbols rather than just deleting it?

I mean with any property symbol or not I guess we could just set it to undefined but it'd be weird that it is there at all imo. Could work though...

Fishrock123 · 2017-03-24T14:40:30Z

Updated to null out the property. ~~New CI: https://ci.nodejs.org/job/node-test-pull-request/7020/~~

@indutny, @misterdjules lgty?

@nodejs/ctc please take a quick look if you could since it is semver-major.

Fishrock123 · 2017-03-24T14:50:48Z

Oops, actually new CI: https://ci.nodejs.org/job/node-test-pull-request/7021/

misterdjules

The following test program:

setTimeout(function foo() {}, 1000);
setTimeout(function foo() {}, 1000).unref();

breaks with this change:

$ cat /var/tmp/test-settimeout-unref.js 
setTimeout(function foo() {}, 1000);
setTimeout(function foo() {}, 1000).unref();
$ ./node /var/tmp/test-settimeout-unref.js 
timers.js:541
    handle._list = null;
                 ^

TypeError: Cannot set property '_list' of null
    at Timeout.unref (timers.js:541:18)
    at Object.<anonymous> (/private/var/tmp/test-settimeout-unref.js:2:37)
    at Module._compile (module.js:607:30)
    at Object.Module._extensions..js (module.js:618:10)
    at Module.load (module.js:516:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Module.runMain (module.js:643:10)
    at run (bootstrap_node.js:444:7)
    at startup (bootstrap_node.js:151:9)
$

and runs successfully without this change.

misterdjules · 2017-04-03T18:59:06Z

lib/timers.js

@@ -538,6 +538,7 @@ Timeout.prototype.unref = function() {
    }

    var handle = reuse(this);
+    handle._list = null;


What happens when handle is null?

Fishrock123 · 2017-04-10T23:52:18Z

Updated, CI again: https://ci.nodejs.org/job/node-test-pull-request/7316/

misterdjules · 2017-04-11T21:32:23Z

test/parallel/test-timers-unref-reuse-no-exposed-list.js

@@ -0,0 +1,16 @@
+'use strict';


Does this need a license block now that #10155 landed?

... No? It did not have previous attribution? (It's new?)

OK, it's been a while I haven't added a file to the repository, and it seems files added in the recent past did not include a license block, so it sounds good to me!

misterdjules · 2017-04-11T21:34:21Z

test/parallel/test-timers-unref-reuse-no-exposed-list.js

+// Check that everything works even if the handle was not re-used.
+setTimeout(() => {}, 1);
+const timer2 = setTimeout(() => {}, 1).unref();
+// Note: It is unlikely that this assertion will be hit in a failure.


That comment seems to be specific to a previous version of this PR where it would throw in that case. I don't think it's relevant in the final version.

Not really, the comment states if that were to fail, i.e. the conditional was removed, node will explode internally rather than triggering this assertion.

Why would "a failure" be specifically that "the conditional was removed"? This is anticipating the future, and not reflecting the current state of the code base. Without the context of what this PR looked like in an intermediate version, this comment doesn't make any sense.

I still think it's not relevant.

I can't think of how this will change without removing handle re-use... Are you saying that a comment that either: might be useful or not matter at all would be better off not being there at all? Isn't that every comment ever?

That bit of code does primarily test that conditional though, that isn't a problem IMO. We have added plenty of test code for code coverage that would be similar.

The test code itself is great, and the comment above:

// Check that everything works even if the handle was not re-used.

is both necessary and sufficient to explain why this test code is present.

On the other hand, I find the comment:

// Note: It is unlikely that this assertion will be hit in a failure.
// Instead, timers.js will likely throw a TypeError internally.

confusing and detrimental to the understanding of that test because it does not reflect the current state of the implementation. Instead it describes what could happen if the code was changed in a specific way, that is if a conditional was removed.

I don't think we want to start documenting every possible way this test could fail depending on an infinite list of possible changes, or by definition we will end up with an infinite list of comments.

misterdjules

A few more comments about the form, otherwise I'm OK wit the nature of the changes.

Fishrock123 · 2017-04-12T14:51:11Z

@misterdjules I believe I have addressed your comments.

@indutny last chance to look, I assume you are not exactly interested so I'll be dismissing your review as it was long since addressed.

Fishrock123 · 2017-04-20T15:44:10Z

@misterdjules updated, lgty?

Fishrock123 · 2017-04-20T15:55:30Z

Here's a CITGM run although I'd be far beyond surprised if we covered a module interacting with this in any way. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/719/

BridgeAR · 2017-08-26T09:26:32Z

@indutny you never responded back and this is blocked because of that. It could land otherwise so it would be nice if you could have a look.

mcollina

LGTM..

can we land this before 9?

@indutny can you review again?

Dismissing the review due to no response. Please take another look.

BridgeAR · 2017-09-20T00:59:22Z

I dismissed the review of @indutny. @nodejs/tsc PTAL because of that. This should be ready to land otherwise.

mcollina · 2017-09-20T05:40:37Z

let’s land this.

jasnell · 2017-09-20T07:51:10Z

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

BridgeAR · 2017-09-20T14:02:32Z

Landed in 11f7dcf

The list is defunct at this point. I believe this to be in effect a minor defect from 3eecdf9 PR-URL: #8422 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

The list is defunct at this point. I believe this to be in effect a minor defect from 3eecdf9 PR-URL: nodejs/node#8422 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Julien Gilli <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 6, 2016

YurySolovyov reviewed Sep 6, 2016
View reviewed changes

Trott mentioned this pull request Sep 9, 2016

Investigate flaky sequential/test-timers-same-timeout-wrong-list-deleted #8459

Closed

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

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

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 5, 2017

indutny previously requested changes Jan 6, 2017

View reviewed changes

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

Fishrock123 force-pushed the timers-fix-list-propagation branch from a64d9fc to 51090c4 Compare March 24, 2017 14:37

targos approved these changes Mar 24, 2017

View reviewed changes

cjihrig approved these changes Mar 24, 2017

View reviewed changes

misterdjules suggested changes Apr 3, 2017

View reviewed changes

[Squash] set _list to undefined

33b213c

Fishrock123 force-pushed the timers-fix-list-propagation branch from 8847f56 to 33b213c Compare April 10, 2017 23:51

misterdjules reviewed Apr 11, 2017

View reviewed changes

misterdjules suggested changes Apr 11, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

[Squash] remove comment

dc49d91

misterdjules approved these changes Apr 20, 2017

View reviewed changes

misterdjules removed the stalled Issues and PRs that are stalled. label Apr 20, 2017

BridgeAR approved these changes Aug 26, 2017

View reviewed changes

mcollina approved these changes Sep 14, 2017

View reviewed changes

jasnell approved these changes Sep 20, 2017

View reviewed changes

BridgeAR closed this Sep 20, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

This was referenced Mar 9, 2024

[Snyk] Upgrade @typescript-eslint/eslint-plugin from 6.21.0 to 7.0.1 B020239/node#2

Open

[Snyk] Upgrade @typescript-eslint/parser from 6.21.0 to 7.0.1 B020239/node#3

Open

timers: do not expose .unref()._handle._list #8422

timers: do not expose .unref()._handle._list #8422

Conversation

Fishrock123 commented Sep 6, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 Sep 6, 2016 • edited Loading

Choose a reason for hiding this comment

misterdjules Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Sep 6, 2016

misterdjules commented Sep 6, 2016

misterdjules commented Sep 12, 2016

Fishrock123 commented Sep 13, 2016

misterdjules commented Sep 13, 2016

Fishrock123 commented Sep 13, 2016

ChALkeR commented Sep 15, 2016 • edited Loading

Fishrock123 commented Jan 5, 2017

targos commented Jan 6, 2017

trevnorris commented Jan 11, 2017

Fishrock123 commented Jan 12, 2017

Fishrock123 commented Jan 12, 2017

jasnell commented Mar 1, 2017

jasnell commented Mar 22, 2017

Fishrock123 commented Mar 22, 2017

Fishrock123 commented Mar 24, 2017 • edited Loading

Fishrock123 commented Mar 24, 2017

misterdjules left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterdjules Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

misterdjules left a comment

Choose a reason for hiding this comment

Fishrock123 commented Apr 12, 2017

Fishrock123 commented Apr 20, 2017

Fishrock123 commented Apr 20, 2017

BridgeAR commented Aug 26, 2017

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 20, 2017

mcollina commented Sep 20, 2017

jasnell commented Sep 20, 2017

BridgeAR commented Sep 20, 2017 • edited Loading

Fishrock123 commented Sep 6, 2016 •

edited

Loading

Fishrock123 Sep 6, 2016 •

edited

Loading

misterdjules Sep 7, 2016 •

edited

Loading

ChALkeR commented Sep 15, 2016 •

edited

Loading

Fishrock123 commented Mar 24, 2017 •

edited

Loading

misterdjules Apr 13, 2017 •

edited

Loading

BridgeAR commented Sep 20, 2017 •

edited

Loading