GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: general improvements to timers.md by jasnell · Pull Request #6937 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented May 23, 2016

Copy link
Copy Markdown
Member
Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc (timers)

Description of change

General improvements to timers.md copy

@nodejs/documentation

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell jasnell added doc Issues and PRs related to Node.js documentation. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout(). labels May 23, 2016
eljefedelrodeodeljefe
eljefedelrodeodeljefe reviewed May 23, 2016
View reviewed changes
Comment thread doc/api/timers.md Outdated
All of the timer functions are globals. You do not need to `require()`
this module in order to use them.
The `timer` module exposes a global API for scheduling callback functions to
execute at some future period of time. Because the timer functions are globals,

@eljefedelrodeodeljefe eljefedelrodeodeljefe May 23, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

execute is "one-off", period would rather indicate a sequence of time, no? Anyhow this sounds a little bulky.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@eljefedelrodeodeljefe

eljefedelrodeodeljefe commented May 23, 2016

Copy link
Copy Markdown
Contributor

Generally LGTM. Good work.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed May 23, 2016
View reviewed changes
Comment thread doc/api/timers.md Outdated
Returns the timer.
Returns a reference to the timer object.

[the Node.js Event Loop]: ../topics/the-event-loop-timers-and-nexttick.html

@addaleax addaleax May 23, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are you sure this works?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell May 23, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not yet. Will be double checking the links before landing.
On May 23, 2016 3:57 PM, "Anna Henningsen" [email protected] wrote:

In doc/api/timers.md
#6937 (comment):

+[the Node.js Event Loop]: ../topics/the-event-loop-timers-and-nexttick.html

Are you sure this works?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6937/files/a8c72718f4500daa613cfe84416f19e8664f6378#r64303631

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Fishrock123 Fishrock123 May 24, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think so

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex

mscdex commented May 24, 2016

Copy link
Copy Markdown
Contributor

/cc @Fishrock123
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed May 24, 2016
View reviewed changes
Comment thread doc/api/timers.md Outdated

By default, when a timer is scheduled using either `setTimeout()` or
`setInterval()`, the Node.js event loop will continue running as long as the
timer is active. Each of the opaque timer objects returned by these functions

@Fishrock123 Fishrock123 May 24, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"opaque" should be something along the lines of "internally tracked" or something

i.e. no access to a _handle

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented May 24, 2016

Copy link
Copy Markdown
Contributor

Should be blocked by #6206 or that should be incorporated, but I'd like to preserve @bengl's contribution if possible.

It should have already landed I think but I haven't gotten around to it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 24, 2016

Copy link
Copy Markdown
Member Author

@bengl ... looking at #6206, how about we combine efforts into a single PR? I can port your edits on top of mine (keeping it as a separate commit with your name attached).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123 Fishrock123 mentioned this pull request May 24, 2016
timers: warn on overflowed timeout duration #6956
Closed
3 tasks done
@bengl

bengl commented May 27, 2016

Copy link
Copy Markdown
Member

@jasnell Yeah sure. You want me to PR against your branch or you got it?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 27, 2016

Copy link
Copy Markdown
Member Author

Either way works!
On May 27, 2016 2:40 PM, "Bryan English" [email protected] wrote:

@jasnell https://github.com/jasnell Yeah sure. You want me to PR
against your branch or you got it?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6937 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eZi4KT-7AqEqhXPOQwPXviks2z5Jks5qF2S6gaJpZM4Ik4sD
.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl bengl mentioned this pull request May 27, 2016
doc: add timer classes jasnell/node#9
Closed
@bengl

bengl commented May 27, 2016

Copy link
Copy Markdown
Member

@jasnell alright I made a PR against your branch with roughly the same changes (it was easier to do a completely commit than to rebase).
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the doc-timers-copy branch from f39fc78 to 3b23032 Compare May 28, 2016 00:03
@jasnell

jasnell commented May 28, 2016

Copy link
Copy Markdown
Member Author

Awesome, thank you. Squashed my commits and pulled yours in.
@Fishrock123 ... LGTY?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented May 28, 2016

Copy link
Copy Markdown
Member Author

@bengl ... made a few additional edits on top .. PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bengl

bengl commented May 28, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the doc-timers-copy branch from dbdd13a to e0ee2df Compare May 28, 2016 01:36
@benjamingr

benjamingr commented May 28, 2016

Copy link
Copy Markdown
Member

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed May 28, 2016
View reviewed changes
Comment thread doc/api/timers.md Outdated

@Fishrock123 Fishrock123 May 28, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Has no effect unless the Timeout was previously unrefed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Fishrock123 Fishrock123 May 28, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Also this does not restore overhead lost by calling unref() -- once you unref it, we'd have to do a linear walk to re-insert it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell May 31, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Neither of these comments seem to be necessary to add to the user documentation in this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Jun 2, 2016

Copy link
Copy Markdown
Member Author

Updated. PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jun 3, 2016

Copy link
Copy Markdown
Member Author

If there are no further comments, I will land on Monday
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed Jun 3, 2016
View reviewed changes
Comment thread doc/api/timers.md Outdated

@Fishrock123 Fishrock123 Jun 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Not incorrect, but I think this is still poorly worded.

When called, the Timeout will not keep the event loop open. When Node.js would exit due to inactivity, this timer will longer be considered as activity, and the program may stop before it calls it's callback.

Maybe that would be better? I agree the wording is hard without having a strong idea of the event loop and handle refedness.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Jun 3, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm not sure that's any better. Rather than hold this up while we get the wording perfect on this, I'd recommend landing and iterating on it separately in subsequent PRs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Fishrock123 Fishrock123 Jun 3, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Hmmm, perhaps, but it should probably at least indicate it has an effect on when the process exits. Perhaps the original wording is the best compromise for now then?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented Jun 3, 2016

Copy link
Copy Markdown
Contributor

LGTM minus nits (particularly the last one)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell mentioned this pull request Jun 6, 2016
doc: more detail on when timers are executed #7166
Closed
2 tasks done
@bengl bengl mentioned this pull request Jun 6, 2016
doc: add timer classes #6206
Closed
2 tasks done
@Fishrock123 Fishrock123 removed their assignment Jun 15, 2016
@Fishrock123

Fishrock123 commented Jun 15, 2016

Copy link
Copy Markdown
Contributor

ping @jasnell
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Jun 15, 2016

Copy link
Copy Markdown
Member Author

Will be back on this after I'm back from vacation
On Jun 15, 2016 1:06 PM, "Jeremiah Senkpiel" [email protected]
wrote:

ping @jasnell https://github.com/jasnell

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6937 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2eaKRtSzCcz4qv6BkCJ3KMN9bk3oUks5qMFtCgaJpZM4Ik4sD
.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the doc-timers-copy branch from b707c46 to 5036973 Compare June 21, 2016 17:30
@jasnell

jasnell commented Jun 21, 2016

Copy link
Copy Markdown
Member Author

Updated one final time to address nit
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fishrock123
Fishrock123 reviewed Jun 27, 2016
View reviewed changes
Comment thread doc/api/globals.md Outdated

@Fishrock123 Fishrock123 Jun 27, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this link is incorrect?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fishrock123

Fishrock123 commented Jun 27, 2016

Copy link
Copy Markdown
Contributor

LGTM, might want to make sure the link is right though. The other bits can always be addressed in a new PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the doc-timers-copy branch from 5036973 to fb05064 Compare June 27, 2016 16:04
@jasnell

jasnell commented Jun 27, 2016

Copy link
Copy Markdown
Member Author

Updated to address the final round of nits
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fishrock123

Fishrock123 commented Jun 28, 2016

Copy link
Copy Markdown
Contributor

Seems good, let's land and unblock #6956
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
doc: general improvements to timers.md
4091f25 
Overall improvements to timers.md documentation,

Includes squashed commit from @bengl:

  doc: add timer classes

  The timers returned by `setTimeout` and friends are
  actually instances of `Timeout` and `Immediate`.
  Documenting them as such, so that the `ref` and
  `unref` methods can be identified as methods on
  `Timeout` objects.

  Sparked by discussion in nodejs#5792
@jasnell
jasnell force-pushed the doc-timers-copy branch from fb05064 to 4091f25 Compare June 28, 2016 14:28
jasnell added a commit that referenced this pull request Jun 28, 2016
@jasnell
doc: general improvements to timers.md
86e07b7 
Overall improvements to timers.md documentation,

Includes squashed commit from @bengl:

  doc: add timer classes

  The timers returned by `setTimeout` and friends are
  actually instances of `Timeout` and `Immediate`.
  Documenting them as such, so that the `ref` and
  `unref` methods can be identified as methods on
  `Timeout` objects.

  Sparked by discussion in #5792

PR-URL: #6937
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@jasnell

jasnell commented Jun 28, 2016

Copy link
Copy Markdown
Member Author

Landed in 86e07b7
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Jun 28, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
@jasnell @Fishrock123
doc: general improvements to timers.md
bc37e6a 
Overall improvements to timers.md documentation,

Includes squashed commit from @bengl:

  doc: add timer classes

  The timers returned by `setTimeout` and friends are
  actually instances of `Timeout` and `Immediate`.
  Documenting them as such, so that the `ref` and
  `unref` methods can be identified as methods on
  `Timeout` objects.

  Sparked by discussion in #5792

PR-URL: #6937
Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Propose v6.3.0 (v2) #7550
Merged
@MylesBorins MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed
@MylesBorins MylesBorins added the dont-land-on-v4.x label Jul 11, 2016
@blacksun1 blacksun1 mentioned this pull request Jan 17, 2017
observed setImmediate and setTimeout execution order contradicts documentation #7145
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. timers Issues and PRs related to timers, setImmediate(), setInterval(), and setTimeout().

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@jasnell @eljefedelrodeodeljefe @mscdex @Fishrock123 @bengl @benjamingr @addaleax @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.