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

src: remove TimerWrap #20894

Closed

Conversation

apapirovski
Copy link
Member

Remove TimerWrap in favour of uv_timer_t handle stored on the Environment, more similar to how Immediates currently function. Since there's now only a single TimerWrap, it doesn't really make much sense for it to exist at all (hence this PR). In addition, the async stack from the TimerWrap is actually not desirable and every single public module that works with async hooks seems to filter it out anyway.

I've also done a quick scan of npm modules and I could not find a single use case of TimerWrap.now(), which would be the only reason to keep it publicly available. It's also currently impossible for user land to use the TimerWrap given how OnTimeout is implemented (it can only process our own internal lists).

This will make it possible to continue further improving our Timers architecture and C++ code.

Fixes: #10154

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@apapirovski apapirovski added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. semver-major PRs that contain breaking changes and should be released in the next major version. labels May 22, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 22, 2018
@apapirovski
Copy link
Member Author

apapirovski commented May 22, 2018

@apapirovski apapirovski force-pushed the patch-timerwrap-refactor branch from 28a7570 to 30c54f9 Compare May 22, 2018 20:50
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Heads up, this is going to have a minor conflict with #20884 :)

src/env.cc Outdated
@@ -450,6 +459,59 @@ void Environment::RunAndClearNativeImmediates() {
}


void Environment::ScheduleTimer(int64_t duration) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the unit of measurement to duration? E.g. duration_ms if it’s milliseconds?

src/env.cc Outdated
do {
TryCatch try_catch(env->isolate());
try_catch.SetVerbose(true);
ret = cb->Call(env->context(), process, 1, args);
Copy link
Member

Choose a reason for hiding this comment

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

tiny style nit: arraysize(args)

src/env.cc Outdated

if (expiry != 0) {
int64_t duration =
abs(expiry) - (uv_now(env->event_loop()) - env->timer_base());
Copy link
Member

Choose a reason for hiding this comment

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

ditto re: adding the unit to the name

(Also, why abs? Can you add a comment?)

src/timers.cc Outdated
result->Set(env->context(), 1, timer_toggle_ref_function).FromJust();
result->Set(env->context(), 2,
env->immediate_info()->fields().GetJSArray()).FromJust();
result->Set(env->context(), 3, imm_toggle_ref_function).FromJust();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for not providing these directly on the binding?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wondered about that. I don't think so?

@apapirovski
Copy link
Member Author

Thanks @addaleax. I've cleaned up and added some detailed comments. Hope this is better :)

@apapirovski apapirovski force-pushed the patch-timerwrap-refactor branch 2 times, most recently from a0b2f82 to dc80758 Compare May 22, 2018 22:12
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

Copy link
Member

Choose a reason for hiding this comment

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

Is there any possibility of userland native modules depending on this? Is it safe to remove without a deprecation cycle?

/cc @bnoordhuis

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done an extensive search and there was not a single module I could find using it. I'm not really sure why someone would want to anyway given that Timer.now() is not that different from process.hrtime() but isn't officially supported.

But also process.binding shouldn't be part of our official deprecation cycle, should it? We broke Gulp in 10.0.0 with changes to contextify and that's a huge module.

(And the TimerWrap itself is impossible to use outside of Node.js.)

@jasnell
Copy link
Member

jasnell commented May 23, 2018

Does this need a proper deprecation cycle? /cc @nodejs/tsc

@apapirovski
Copy link
Member Author

apapirovski commented May 23, 2018

@apapirovski apapirovski force-pushed the patch-timerwrap-refactor branch from dc80758 to 15b2ddd Compare May 23, 2018 04:03
@@ -52,7 +52,7 @@ async function test() {
'node.perf.timerify', 'v8'],
categories);

const traceConfig = { includedCategories: ['node'] };
const traceConfig = { includedCategories: ['v8'] };
Copy link
Member Author

Choose a reason for hiding this comment

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

@eugeneo This test fails without this change. I suspect it has to do with the removal of the HandleWrap, right? Or is there something else at play? Would appreciate you having a look as the author of this test and someone with a lot of understanding of all this tracing stuff. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@nodejs/diagnostics Perhaps some of you might have insight too? Thanks!

This test fails without this change. I suspect it has to do with the removal of the HandleWrap, right? Or is there something else at play? Would appreciate you having a look as the author of this test and someone with a lot of understanding of all this tracing stuff. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

It's because timer_wrap.cc uses AsyncWrap::MakeCallback(), which calls AsyncWrap::EmitTraceEventAfter(). Since you no longer use that method, it's not traced any more.

I personally don't feel too strongly but I guess it would be nice to keep the current behavior.

Copy link
Member Author

@apapirovski apapirovski May 23, 2018

Choose a reason for hiding this comment

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

I wonder if we should do something about Immediates too in that case? But I don't know how to best handle it since there's no longer an AsyncWrap (or obviously a matching provider). All of this trace event stuff is pretty outside of my expertise as far as Node goes so I'm all ears...

Since this is semver-major (and won't land in a release for a while), perhaps we could add a TODO: or start an issue so that more qualified people can chime in (including yourself)?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be okay with that. In my opinion, AsyncWrap currently conflates too much (mushes too much into a single class) and could use some decoupling.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM on the test change. I haven't reviewed the rest of the PR.

@apapirovski apapirovski force-pushed the patch-timerwrap-refactor branch 2 times, most recently from 5ab8e99 to 2b7ae97 Compare May 23, 2018 07:16
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

lib/timers.js Outdated
@@ -53,8 +58,9 @@ const kCount = 0;
const kRefCount = 1;
const kHasOutstanding = 2;

const [immediateInfo, toggleImmediateRef] =
setupTimers(processImmediate, processTimers);
// Call into C++ to assing callbacks that are responsible for processing
Copy link
Member

Choose a reason for hiding this comment

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

I suspect you intended to write 'assign.' :-)

listOnTimeout(list, now);
ran = true;
Copy link
Member

Choose a reason for hiding this comment

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

This is a functionally null change, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's just cleaner this way since we don't assign each time.

src/env.cc Outdated
@@ -161,6 +163,9 @@ void Environment::Start(int argc,
HandleScope handle_scope(isolate());
Context::Scope context_scope(context());

uv_timer_init(event_loop(), timer_handle());
Copy link
Member

Choose a reason for hiding this comment

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

Can you CHECK_EQ(0, uv_timer_init(...));?

(Not that it can actually fail but that's why it should be CHECK'd.)

src/env.cc Outdated

Local<Function> cb = env->timers_callback_function();
MaybeLocal<Value> ret;
Local<Value> args[] = { env->GetNow() };
Copy link
Member

Choose a reason for hiding this comment

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

If you're just passing a single argument you can do Local<Value> arg = env->GetNow() and then call into JS with cb->Call(..., 1, &arg).

Not that this is wrong but it's a bit more obvious at the call site that you're just passing one arg.

src/env.cc Outdated
TryCatch try_catch(env->isolate());
try_catch.SetVerbose(true);
ret = cb->Call(env->context(), process, arraysize(args), args);
} while (ret.IsEmpty());
Copy link
Member

Choose a reason for hiding this comment

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

I realize you brought this over from timer_wrap.cc but is there a reason you can't handle exceptions in JS land?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to let it go to uncaughtException or domain first. I don't think there's a way to do so in JS?

src/env.cc Outdated

if (expiry_ms != 0) {
int64_t duration_ms =
abs(expiry_ms) - (uv_now(env->event_loop()) - env->timer_base());
Copy link
Member

Choose a reason for hiding this comment

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

Either check that expiry_ms fits in an int or use llabs() (or manually turn it positive.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yeah... didn't even realize!

src/env.cc Outdated
}
} else if (has_ref) {
uv_unref(h);
}
Copy link
Member

Choose a reason for hiding this comment

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

uv_ref() and uv_unref() are idempotent, you don't need to check if the timer is ref'd or not.

src/timers.cc Outdated
#include <stdint.h>

namespace node {
namespace timers {
Copy link
Member

Choose a reason for hiding this comment

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

You could also use an anonymous namespace here.

@@ -52,7 +52,7 @@ async function test() {
'node.perf.timerify', 'v8'],
categories);

const traceConfig = { includedCategories: ['node'] };
const traceConfig = { includedCategories: ['v8'] };
Copy link
Member

Choose a reason for hiding this comment

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

It's because timer_wrap.cc uses AsyncWrap::MakeCallback(), which calls AsyncWrap::EmitTraceEventAfter(). Since you no longer use that method, it's not traced any more.

I personally don't feel too strongly but I guess it would be nice to keep the current behavior.

const N = 30;

let last_i = 0;
let last_ts = 0;
const [seconds, nanoseconds] = process.hrtime();
let last_ts = (seconds * 1e3 + nanoseconds * 1e-6) | 0;
Copy link
Member

Choose a reason for hiding this comment

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

Not sound. process.hrtime() returns the number of nanoseconds since an unspecified point in the past. If that point is the UNIX epoch then this stops working sometime next year because seconds * 1e3 >= 2**32.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, sounds like it's best to expose getLibuvNow() from internal/timers and just use that then.

@apapirovski
Copy link
Member Author

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the investigation on the semveriness :-)

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 24, 2018
@apapirovski
Copy link
Member Author

Just an FYI, while this is marked as "author ready" and could land, I'm hoping to have @Fishrock123 review it. Please don't land until they either review or say they won't have time to do so.

@Fishrock123
Copy link
Contributor

I’m on Vacation. It’l have to wait until during or after JSConf.EU.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

maclover7 added a commit that referenced this pull request Jul 15, 2018
Unused since the excellent refactoring in 2930bd,
which also removed `src/timer_wrap.cc` from `node.gyp`. If you try and
get at the binding on a v11.0-pre build, you'll get an error, since the
file is no longer in the GYP build.

```
Jonathans-MBP:node jon$ ./node -v
v11.0.0-pre
Jonathans-MBP:node jon$ ./node -e "process.binding('timer_wrap')"
internal/bootstrap/loaders.js:81
        mod = bindingObj[module] = getBinding(module);
                                   ^

Error: No such module: timer_wrap
    at process.binding (internal/bootstrap/loaders.js:81:36)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:89:20)
    at Object.runInThisContext (vm.js:286:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at evalScript (internal/bootstrap/node.js:562:27)
    at startup (internal/bootstrap/node.js:248:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)
```

PR-URL: #21777
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #20894
targos pushed a commit to targos/node that referenced this pull request Jul 31, 2018
The timers directory test, utilizing FakeTime, has not worked in
quite a while and is not truly testing Node.js behaviour. If a
similar test is necessary it would be better suited to libuv
on which Node.js relies for timers functionality.

PR-URL: nodejs#20894
Fixes: nodejs#10154
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos pushed a commit that referenced this pull request Aug 2, 2018
The timers directory test, utilizing FakeTime, has not worked in
quite a while and is not truly testing Node.js behaviour. If a
similar test is necessary it would be better suited to libuv
on which Node.js relies for timers functionality.

PR-URL: #20894
Fixes: #10154
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>

Backport-PR-URL: #22039
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
rvagg added a commit to rvagg/io.js that referenced this pull request Aug 13, 2018
nodejs#20894 / 2930bd1 was introduced on
master which removed an offending line in this doc before linting was
applied to test/ in nodejs#22221 /
56103ab. Since 20894 is semver-major, the full changes were not
backported.
rvagg added a commit that referenced this pull request Aug 13, 2018
#20894 / 2930bd1 was introduced on
master which removed an offending line in this doc before linting was
applied to test/ in #22221 /
56103ab. Since 20894 is semver-major, the full changes were not
backported.

PR-URL: #22296
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
rvagg added a commit that referenced this pull request Aug 15, 2018
#20894 / 2930bd1 was introduced on
master which removed an offending line in this doc before linting was
applied to test/ in #22221 /
56103ab. Since 20894 is semver-major, the full changes were not
backported.

PR-URL: #22296
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
vsemozhetbyt pushed a commit that referenced this pull request Nov 11, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in nodejs#20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: nodejs#24050
Refs: nodejs#20894
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Dec 14, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setTimeout callback executed before delay elapses