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: make timeout.refresh() a public API #20298

Closed
wants to merge 1 commit into from

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Apr 25, 2018

Originally added in bb5575a discussions such as #20261 show the usefulness of this API to the Node.js ecosystem.

cc @nodejs/timers

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

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

Note to self: Reword the commit to be timeout.refresh()

@Fishrock123 Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 25, 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 Apr 25, 2018
@Fishrock123 Fishrock123 changed the title timers: make timer.refresh() a public API timers: make timeout.refresh() a public API Apr 25, 2018
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 25, 2018

Sets the timer's start time to the current time, and reschedules the timer to
call its callback at the previously specified duration adjusted to the current
time. This is useful for refreshing a timeout without allocating a new
Copy link
Member

Choose a reason for hiding this comment

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

Nit/question: Should timeout be timer in this sentence? Basically, if it refers to the same thing as timer in the previous sentence, let's pick one word and stick with it. If it refers to something different, and the definitions are not clear elsewhere in the doc, let's add them?

@@ -61,6 +61,21 @@ timer is active. Each of the `Timeout` objects returned by these functions
export both `timeout.ref()` and `timeout.unref()` functions that can be used to
control this default behavior.

### timeout.refresh()
Copy link
Member

Choose a reason for hiding this comment

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

The methods should be arranged alphabetically, so this entry should come after timeout.ref(), not before it.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Couple of small doc nits, but LGTM otherwise.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM. The only question is whether we want to make it refreshTimeout(timeout) in case the standards people ever want to add similar functionality? (Since it's IMO useful.)

@@ -82,7 +80,7 @@ function Timeout(callback, after, args, isRepeat, isUnrefed) {
initAsyncResource(this, 'Timeout');
}

Timeout.prototype[refreshFnSymbol] = function refresh() {
Timeout.prototype.refresh = function refresh() {
Copy link
Member

Choose a reason for hiding this comment

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

This function doesn't need to be named. The name that V8 generates is nicer.

time. This is useful for refreshing a timeout without allocating a new
JavaScript object.

Using this on a timer that has already called its callback **will reactivate**
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs the emphasis.

@benjamingr
Copy link
Member

This diverges further from browsers do -

I'm also not sure what purpose this serves over creating a new timeout? Can someone explain?

@apapirovski
Copy link
Member

This diverges further from browsers do -

I don't really think it does. We already return an object. Adding more methods at this point doesn't make a difference. (Or a helper function like refreshTimeout.) None of this is incompatible with the browsers.

I'm also not sure what purpose this serves over creating a new timeout? Can someone explain?

Performance. clearTimeout + setTimeout back to back are slow.

@benjamingr
Copy link
Member

Performance. clearTimeout + setTimeout back to back are slow.

Can't we fix that?

@apapirovski
Copy link
Member

apapirovski commented Apr 25, 2018

Can't we fix that?

Not really. clearTimeout could be removing the last timeout in a list, deleting that list, closing a libuv handle, then creating a timeout for the same duration, creating that list and opening a new handle.

Even ignoring that, it's still double the work to mark a timeout as disposed, free it up for GC, allocate a new timeout, enlist it, etc.


There's a reason we used to use enroll. @Fishrock123 added the private refresh method to allow us to get rid of enroll and not sacrifice performance.

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Apr 25, 2018

I'm also not sure what purpose this serves over creating a new timeout? Can someone explain?

Less memory pressure for persistent connections with timeouts. It is without question useful - we do this internally, and the ecosystem also does it because of that. Also, this functionality is technically already available, but in a way that should be deprecated after this. (Timers.active())

(Browsers don't care for having many connections open, so this doesn't really impact them as much anyways.)

@benjamingr
Copy link
Member

Thanks for clarifying.

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 26, 2018
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

I've proposed some tiny changes to the returned values system in this doc: #20310 If that PR is landed, it would be good to rebase and conform)

@addaleax
Copy link
Member

A bit confused about the labels – @Fishrock123 This doesn’t target the v10.x branch?

@BridgeAR I think adding the author ready label when there’s unresolved discussion is not ideal, and is one of the reasons that led to the discussions in openjs-foundation/summit#64. It’s nice for PRs to land early, but not everybody has the response times that you and I can afford ourselves.

(@benjamingr If you do think that this doesn’t need further discussion, feel free to say so/re-add the label.)

@addaleax addaleax removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. v10.x labels Apr 27, 2018
@benjamingr
Copy link
Member

We're debating on adding this to lolex in sinonjs/fake-timers#187 ^

If anyone has strong feelings about what we should do - LMK. lolex is pretty widely used (everyone using sinon and soon jest).

MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
MylesBorins pushed a commit that referenced this pull request May 23, 2018
* addons:
  - Fixed a memory leak for users of `AsyncResource` and N-API.
    (Michael Dawson)
    #20668
* assert:
  - The `error` parameter of `assert.throws()` can be an object containing
    regular expressions now. (Ruben Bridgewater)
    #20485
* crypto:
  - The `authTagLength` option has been made more flexible (Tobias Nießen)
    #20235)
    #20039
* esm:
  - Builtin modules (e.g. `fs`) now provide named exports in ES6 modules.
    (Gus Caplan)
    #20403
* http:
  - Handling of `close` and `aborted` events has been made more consistent.
    (Robert Nagy)
    #20075
    #20611
* module:
  - add --preserve-symlinks-main (David Goldstein)
    #19911
* timers:
  - `timeout.refresh()` has been added to the public API.
    (Jeremiah Senkpiel)
    #20298
* Embedder support:
  - Functions for creating V8 `Isolate` and `Context` objects with
    Node.js-specific behaviour have been added to the API.
    (Allen Yonghuang Wang)
    #20639
  - Node.js `Environment`s clean up resources before exiting now.
    (Anna Henningsen)
    #19377
  - Support for multi-threaded embedding has been improved.
    (Anna Henningsen)
    #20542
    #20539
    #20541

PR-URL: #20724
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 19, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: nodejs#18066
Refs: nodejs#20298

PR-URL: nodejs#26760
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 23, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: nodejs#18066
Refs: nodejs#20298

PR-URL: nodejs#26760

Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Mar 28, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: #18066
Refs: #20298

PR-URL: #26760

Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Mar 30, 2019
Another nail in the coffin here, farewell ye ol C-style apis.

These apis caused numerous other issues that required far too many
safeguards. This gets us one step closer to not having to worry about
those issues anymore.

Refs: #18066
Refs: #20298

PR-URL: #26760

Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor 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.