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

timers: reuse timer in `setTimeout().unref()` #3407

Closed
wants to merge 1 commit into from

Conversation

@indutny
Copy link
Member

@indutny indutny commented Oct 16, 2015

Instead of creating new timer - reuse the timer from the freelist. This
won't make the freelist timer active for the duration of uv_close(),
and will let the event-loop exit properly.

Fix: #1264

R=@trevnorris @Fishrock123

Instead of creating new timer - reuse the timer from the freelist. This
won't make the freelist timer active for the duration of `uv_close()`,
and will let the event-loop exit properly.

Fix: #1264
@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Oct 16, 2015

LGTM

May want @bnoordhuis to take a quick look?

@trevnorris trevnorris added the timers label Oct 16, 2015
@indutny
Copy link
Member Author

@indutny indutny commented Oct 16, 2015

Sure.

@indutny
Copy link
Member Author

@indutny indutny commented Oct 16, 2015

Looks like CI is green.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Oct 16, 2015

I'd like to take a look at this later before it lands. ✈️ Right now.

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Oct 16, 2015

Is this something that could be merged into LTS?

Seems like it might be but perhaps I'm missing some subtlety
@jasnell

@indutny
Copy link
Member Author

@indutny indutny commented Oct 16, 2015

I think we may want to have it in LTS, indeed.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Oct 19, 2015

Will take a look today.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Oct 19, 2015

Hmmm, I think this underlines a problem with how beforeExit works, not timers. I think this is just circumventing the issue.

@indutny
Copy link
Member Author

@indutny indutny commented Oct 19, 2015

@Fishrock123 I would say a problem with how beforeExit is documented, as is supposed to work. We should definitely look into fixing some stuff at libuv level, once @saghul will be back.

This is fix should be enough for now to handle the edge case between windows/unix.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Oct 20, 2015

@indutny if I am understanding this properly, the problem is that the original list handle registers as active during the event loop where it is first closed. Is that correct?

If so, is there any disadvantage to unrefing the original handle instead of this in that case?

@trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Oct 20, 2015

@Fishrock123 Then you'd have to check if the handle has any ref'd timers remaining. Accomplishing most of what this patch does. Difference is this one saves an allocation.

@indutny
Copy link
Member Author

@indutny indutny commented Oct 20, 2015

@Fishrock123 it won't help to unref it after closing, because libuv checks for CLOSING flag before decrementing the active handle count.

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Oct 20, 2015

LGTM I suppose, but I don't understand how this actually makes a difference.

@indutny
Copy link
Member Author

@indutny indutny commented Oct 20, 2015

@Fishrock123 uv_close makes timer active on windows, this adds extra beforeExit. With this commit, we won't close the handle, and will instead reuse it.

@indutny
Copy link
Member Author

@indutny indutny commented Oct 20, 2015

Landed in 3eecdf9, thank you!

@indutny indutny closed this Oct 20, 2015
indutny added a commit that referenced this pull request Oct 20, 2015
Instead of creating new timer - reuse the timer from the freelist. This
won't make the freelist timer active for the duration of `uv_close()`,
and will let the event-loop exit properly.

Fix: #1264
PR-URL: #3407
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@indutny indutny deleted the indutny:fix/gh-1264 branch Oct 20, 2015
indutny added a commit that referenced this pull request Oct 21, 2015
Instead of creating new timer - reuse the timer from the freelist. This
won't make the freelist timer active for the duration of `uv_close()`,
and will let the event-loop exit properly.

Fix: #1264
PR-URL: #3407
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@rvagg rvagg mentioned this pull request Oct 21, 2015
indutny added a commit that referenced this pull request Oct 28, 2015
Instead of creating new timer - reuse the timer from the freelist. This
won't make the freelist timer active for the duration of `uv_close()`,
and will let the event-loop exit properly.

Fix: #1264
PR-URL: #3407
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@jasnell
Copy link
Member

@jasnell jasnell commented Oct 28, 2015

Landed in v4.x-staging in 40bb802

@jasnell jasnell removed the lts-watch-v4.x label Oct 28, 2015
rvagg added a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
Notable changes:

* buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer,
  these have been deprecated for a long time (Sakthipriyan Vairamani) nodejs#2859.
* console: (Breaking) Values reported by console.time() now have 3 decimals of
  accuracy added (Michaël Zasso) nodejs#3166.
* fs:
  - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file
    descriptor as their first argument (Johannes Wüller) nodejs#3163.
  - (Breaking) In fs.readFile(), if an encoding is specified and the internal
    toString() fails the error is no longer thrown but is passed to the callback
    (Evan Lucas) nodejs#3485.
  - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding,
    callback) form), if the internal toString() fails the error is no longer
    thrown but is passed to the callback (Evan Lucas) nodejs#3503.
* http:
  - Fixed a bug where pipelined http requests would stall (Fedor Indutny) nodejs#3342.
  - (Breaking) When parsing HTTP, don't add duplicates of the following headers:
    Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition
    to the following headers which already block duplicates: Content-Type,
    Content-Length, User-Agent, Referer, Host, Authorization,
    Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location,
    Max-Forwards (James M Snell) nodejs#3090.
  - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a
    function or a TypeError is thrown (James M Snell) nodejs#3090.
  - (Breaking) HTTP methods and header names must now conform to the RFC 2616
    "token" rule, a list of allowed characters that excludes control characters
    and a number of separator characters. Specifically, methods and header names
    must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown
    (James M Snell) nodejs#2526.
* node:
  - (Breaking) Deprecated the _linklist module (Rich Trott) nodejs#3078.
  - (Breaking) Removed require.paths and require.registerExtension(), both had
    been previously set to throw Error when accessed
    (Sakthipriyan Vairamani) nodejs#2922.
* npm: Upgraded to version 3.3.6 from 2.14.7, see
  https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a
  major version bump for npm and it has seen a significant amount of change.
  Please see the original npm v3.0.0 release notes for a list of major changes
  (Rebecca Turner) nodejs#3310.
* src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary
  due to the V8 upgrade. Native add-ons will need to be recompiled
  (Rod Vagg) nodejs#3400.
* timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes
  a long-standing known issue where unrefed timers would perviously hold
  beforeExit open (Fedor Indutny) nodejs#3407.
* tls:
  - Added ALPN Support (Shigeki Ohtsu) nodejs#2564.
  - TLS options can now be passed in an object to createSecurePair()
    (Коренберг Марк) nodejs#2441.
  - (Breaking) The default minimum DH key size for tls.connect() is now 1024
    bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS
    option can be used to override the default. (Shigeki Ohtsu) nodejs#1831.
* util:
  - (Breaking) util.p() was deprecated for years, and has now been removed
    (Wyatt Preul) nodejs#3432.
  - (Breaking) util.inherits() can now work with ES6 classes. This is considered
    a breaking change because of potential subtle side-effects caused by a
    change from directly reassigning the prototype of the constructor using
    `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })`
    to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)`
    (Michaël Zasso) nodejs#3455.
* v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) nodejs#3351.
  - Implements the spread operator, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
    for further information.
  - Implements new.target, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target
    for further information.
* zlib: Decompression now throws on truncated input (e.g. unexpected end of
  file) (Yuval Brik) nodejs#2595.

PR-URL: nodejs#3466
indutny added a commit that referenced this pull request Oct 29, 2015
Instead of creating new timer - reuse the timer from the freelist. This
won't make the freelist timer active for the duration of `uv_close()`,
and will let the event-loop exit properly.

Fix: #1264
PR-URL: #3407
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg added a commit that referenced this pull request Oct 29, 2015
Notable changes:

* buffer: (Breaking) Removed both 'raw' and 'raws' encoding types from Buffer,
  these have been deprecated for a long time (Sakthipriyan Vairamani) #2859.
* console: (Breaking) Values reported by console.time() now have 3 decimals of
  accuracy added (Michaël Zasso) #3166.
* fs:
  - fs.readFile*(), fs.writeFile*(), and fs.appendFile*() now also accept a file
    descriptor as their first argument (Johannes Wüller) #3163.
  - (Breaking) In fs.readFile(), if an encoding is specified and the internal
    toString() fails the error is no longer thrown but is passed to the callback
    (Evan Lucas) #3485.
  - (Breaking) In fs.read() (using the fs.read(fd, length, position, encoding,
    callback) form), if the internal toString() fails the error is no longer
    thrown but is passed to the callback (Evan Lucas) #3503.
* http:
  - Fixed a bug where pipelined http requests would stall (Fedor Indutny) #3342.
  - (Breaking) When parsing HTTP, don't add duplicates of the following headers:
    Retry-After, ETag, Last-Modified, Server, Age, Expires. This is in addition
    to the following headers which already block duplicates: Content-Type,
    Content-Length, User-Agent, Referer, Host, Authorization,
    Proxy-Authorization, If-Modified-Since, If-Unmodified-Since, From, Location,
    Max-Forwards (James M Snell) #3090.
  - (Breaking) The callback argument to OutgoingMessage#setTimeout() must be a
    function or a TypeError is thrown (James M Snell) #3090.
  - (Breaking) HTTP methods and header names must now conform to the RFC 2616
    "token" rule, a list of allowed characters that excludes control characters
    and a number of separator characters. Specifically, methods and header names
    must now match /^[a-zA-Z0-9_!#$%&'*+.^`|~-]+$/ or a TypeError will be thrown
    (James M Snell) #2526.
* node:
  - (Breaking) Deprecated the _linklist module (Rich Trott) #3078.
  - (Breaking) Removed require.paths and require.registerExtension(), both had
    been previously set to throw Error when accessed
    (Sakthipriyan Vairamani) #2922.
* npm: Upgraded to version 3.3.6 from 2.14.7, see
  https://github.com/npm/npm/releases/tag/v3.3.6 for more details. This is a
  major version bump for npm and it has seen a significant amount of change.
  Please see the original npm v3.0.0 release notes for a list of major changes
  (Rebecca Turner) #3310.
* src: (Breaking) Bumped NODE_MODULE_VERSION to 47 from 46, this is necessary
  due to the V8 upgrade. Native add-ons will need to be recompiled
  (Rod Vagg) #3400.
* timers: Attempt to reuse the timer handle for setTimeout().unref(). This fixes
  a long-standing known issue where unrefed timers would perviously hold
  beforeExit open (Fedor Indutny) #3407.
* tls:
  - Added ALPN Support (Shigeki Ohtsu) #2564.
  - TLS options can now be passed in an object to createSecurePair()
    (Коренберг Марк) #2441.
  - (Breaking) The default minimum DH key size for tls.connect() is now 1024
    bits and a warning is shown when DH key size is less than 2048 bits. This a security consideration to prevent "logjam" attacks. A new minDHSize TLS
    option can be used to override the default. (Shigeki Ohtsu) #1831.
* util:
  - (Breaking) util.p() was deprecated for years, and has now been removed
    (Wyatt Preul) #3432.
  - (Breaking) util.inherits() can now work with ES6 classes. This is considered
    a breaking change because of potential subtle side-effects caused by a
    change from directly reassigning the prototype of the constructor using
    `ctor.prototype = Object.create(superCtor.prototype, { constructor: { ... } })`
    to using `Object.setPrototypeOf(ctor.prototype, superCtor.prototype)`
    (Michaël Zasso) #3455.
* v8: (Breaking) Upgraded to 4.6.85.25 from 4.5.103.35 (Ali Ijaz Sheikh) #3351.
  - Implements the spread operator, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
    for further information.
  - Implements new.target, see
    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/new.target
    for further information.
* zlib: Decompression now throws on truncated input (e.g. unexpected end of
  file) (Yuval Brik) #2595.

PR-URL: #3466
@jasnell jasnell removed the land-on-v4.x label Nov 3, 2015
@saghul
Copy link
Member

@saghul saghul commented Nov 4, 2015

@Fishrock123 I would say a problem with how beforeExit is documented, as is supposed to work. We should definitely look into fixing some stuff at libuv level, once @saghul will be back.

This is fix should be enough for now to handle the edge case between windows/unix.

@indutny I'm back! Can you open an issue at libuv/libuv please? Bonus points for a test which passes on Unix and fails on Windows :-)

@shigeki shigeki mentioned this pull request Mar 2, 2017
3 of 3 tasks complete
shigeki added a commit to shigeki/node that referenced this pull request Mar 8, 2017
The timer handle is reused when it is unrefed in order to avoid new
callback in beforeExit(nodejs#3407). If it is unrefed within a setInterval
callback, the reused timer handle is closed so that setInterval no
longer keep working. This fix does not close the handle in case of
setInterval.
jasnell added a commit that referenced this pull request Mar 22, 2017
The timer handle is reused when it is unrefed in order to avoid new
callback in beforeExit(#3407). If it is unrefed within a setInterval
callback, the reused timer handle is closed so that setInterval no
longer keep working. This fix does not close the handle in case of
setInterval.

PR-URL: #11646
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Mar 28, 2017
The timer handle is reused when it is unrefed in order to avoid new
callback in beforeExit(#3407). If it is unrefed within a setInterval
callback, the reused timer handle is closed so that setInterval no
longer keep working. This fix does not close the handle in case of
setInterval.

PR-URL: #11646
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Apr 18, 2017
The timer handle is reused when it is unrefed in order to avoid new
callback in beforeExit(#3407). If it is unrefed within a setInterval
callback, the reused timer handle is closed so that setInterval no
longer keep working. This fix does not close the handle in case of
setInterval.

PR-URL: #11646
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Apr 19, 2017
The timer handle is reused when it is unrefed in order to avoid new
callback in beforeExit(#3407). If it is unrefed within a setInterval
callback, the reused timer handle is closed so that setInterval no
longer keep working. This fix does not close the handle in case of
setInterval.

PR-URL: #11646
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.