Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

lib: fix guard expression in timer.unref() #165

Merged
merged 1 commit into from
Dec 18, 2014

Conversation

bnoordhuis
Copy link
Member

Fixes the following assertion on slow systems, like our ARM buildbot:

$ out/Debug/node test/simple/test-timers-unref.js
node: ../src/async-wrap-inl.h:101: v8::Handle<v8::Value>
node::AsyncWrap::MakeCallback(uint32_t, int,
v8::Handle<v8::Value>*): Assertion `cb_v->IsFunction()' failed.
Aborted

The reason it only manifests on slow systems is that the test starts
a 1 ms interval timer, then defers timer.unref.bind({}) to the next
tick. On fast systems, the test completes in under a millisecond,
before the callback is called.

This commit makes timer.unref() check that the receiver actually has
a timeout callback property.

Fixes #13.

R=@rvagg

@rvagg
Copy link
Member

rvagg commented Dec 17, 2014

@bnoordhuis can we craft a test that would simulate the same behaviour on all systems so we can catch regressions more reliably in the future?

@bnoordhuis bnoordhuis force-pushed the fix-arm-buildbot-failure branch from 8069684 to 48989f1 Compare December 17, 2014 13:54
@bnoordhuis
Copy link
Member Author

@rvagg Added a test.

@rvagg
Copy link
Member

rvagg commented Dec 17, 2014

lgtm

Fixes the following assertion on slow systems, like our ARM buildbot:

    $ out/Debug/node test/simple/test-timers-unref.js
    node: ../src/async-wrap-inl.h:101: v8::Handle<v8::Value>
    node::AsyncWrap::MakeCallback(uint32_t, int,
    v8::Handle<v8::Value>*): Assertion `cb_v->IsFunction()' failed.
    Aborted

The reason it only manifests on slow systems is that the test starts
a 1 ms interval timer, then defers timer.unref.bind({}) to the next
tick.  On fast systems, the test completes in under a millisecond,
before the callback is called.

This commit makes timer.unref() check that the receiver actually has
a timeout callback property.

Fixes nodejs#13.

PR-URL: nodejs#165
Reviewed-By: Rod Vagg <rod@vagg.org>
@bnoordhuis bnoordhuis force-pushed the fix-arm-buildbot-failure branch from 48989f1 to ebf9f29 Compare December 18, 2014 17:55
@bnoordhuis bnoordhuis merged commit ebf9f29 into nodejs:v0.12 Dec 18, 2014
@bnoordhuis bnoordhuis deleted the fix-arm-buildbot-failure branch December 18, 2014 17:55
enricogior pushed a commit to thaliproject/jxcore that referenced this pull request Mar 3, 2017
Fixes the following assertion on slow systems, like our ARM buildbot:

    $ out/Debug/node test/simple/test-timers-unref.js
    node: ../src/async-wrap-inl.h:101: v8::Handle<v8::Value>
    node::AsyncWrap::MakeCallback(uint32_t, int,
    v8::Handle<v8::Value>*): Assertion `cb_v->IsFunction()' failed.
    Aborted

The reason it only manifests on slow systems is that the test starts
a 1 ms interval timer, then defers timer.unref.bind({}) to the next
tick.  On fast systems, the test completes in under a millisecond,
before the callback is called.

This commit makes timer.unref() check that the receiver actually has
a timeout callback property.

Fixes #13.

PR-URL: nodejs/node#165
Reviewed-By: Rod Vagg <rod@vagg.org>
enricogior pushed a commit to thaliproject/jxcore that referenced this pull request Mar 4, 2017
Fixes the following assertion on slow systems, like our ARM buildbot:

    $ out/Debug/node test/simple/test-timers-unref.js
    node: ../src/async-wrap-inl.h:101: v8::Handle<v8::Value>
    node::AsyncWrap::MakeCallback(uint32_t, int,
    v8::Handle<v8::Value>*): Assertion `cb_v->IsFunction()' failed.
    Aborted

The reason it only manifests on slow systems is that the test starts
a 1 ms interval timer, then defers timer.unref.bind({}) to the next
tick.  On fast systems, the test completes in under a millisecond,
before the callback is called.

This commit makes timer.unref() check that the receiver actually has
a timeout callback property.

Fixes #13.

PR-URL: nodejs/node#165
Reviewed-By: Rod Vagg <rod@vagg.org>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants