doc: general improvements to timers.md by jasnell · Pull Request #6937 · nodejs/node

jasnell · 2016-05-23T21:13:33Z

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

eljefedelrodeodeljefe · 2016-05-23T21:28:52Z

-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,


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

eljefedelrodeodeljefe · 2016-05-23T21:29:43Z

Generally LGTM. Good work.

addaleax · 2016-05-23T22:57:30Z

-Returns the timer.
+Returns a reference to the timer object.

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


Are you sure this works?

Not yet. Will be double checking the links before landing.
On May 23, 2016 3:57 PM, "Anna Henningsen" notifications@github.com 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

I don't think so

mscdex · 2016-05-24T06:57:22Z

/cc @Fishrock123

Fishrock123 · 2016-05-24T14:04:40Z

+
+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


"opaque" should be something along the lines of "internally tracked" or something
i.e. no access to a _handle

Fishrock123 · 2016-05-24T14:06:31Z

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.

jasnell · 2016-05-24T15:15:30Z

@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).

bengl · 2016-05-27T21:40:03Z

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

jasnell · 2016-05-27T21:46:30Z

Either way works!
On May 27, 2016 2:40 PM, "Bryan English" notifications@github.com 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
.

bengl · 2016-05-27T22:27:27Z

@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 · 2016-05-28T00:03:56Z

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

jasnell · 2016-05-28T00:09:47Z

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

bengl · 2016-05-28T01:35:41Z

LGTM

benjamingr · 2016-05-28T07:27:10Z

LGTM

Fishrock123 · 2016-05-28T14:52:34Z

Has no effect unless the Timeout was previously unrefed.

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.

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

jasnell · 2016-06-02T23:16:34Z

Updated. PTAL

jasnell · 2016-06-03T19:47:28Z

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

Fishrock123 · 2016-06-03T20:01:46Z

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.

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

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?

Fishrock123 · 2016-06-03T20:03:22Z

LGTM minus nits (particularly the last one)

Fishrock123 · 2016-06-15T20:06:19Z

ping @jasnell

jasnell · 2016-06-15T20:22:27Z

Will be back on this after I'm back from vacation
On Jun 15, 2016 1:06 PM, "Jeremiah Senkpiel" notifications@github.com
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
.

jasnell · 2016-06-21T17:30:59Z

Updated one final time to address nit

Fishrock123 · 2016-06-27T12:06:12Z

I think this link is incorrect?

Fishrock123 · 2016-06-27T12:11:38Z

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

jasnell · 2016-06-27T16:04:40Z

Updated to address the final round of nits

Fishrock123 · 2016-06-28T10:00:38Z

Seems good, let's land and unblock #6956

@bengl

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

@bengl

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 <robert.lindstaedt@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell · 2016-06-28T14:40:45Z

Landed in 86e07b7

@bengl

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 <robert.lindstaedt@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell added doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels May 23, 2016

eljefedelrodeodeljefe reviewed May 23, 2016
View reviewed changes

addaleax reviewed May 23, 2016
View reviewed changes

Fishrock123 reviewed May 24, 2016
View reviewed changes

Fishrock123 mentioned this pull request May 24, 2016
timers: warn on overflowed timeout duration #6956
Closed
3 tasks

bengl mentioned this pull request May 27, 2016
doc: add timer classes jasnell/node#9
Closed

jasnell force-pushed the doc-timers-copy branch from f39fc78 to 3b23032Compare May 28, 2016 00:03

jasnell force-pushed the doc-timers-copy branch from dbdd13a to e0ee2dfCompare May 28, 2016 01:36

Fishrock123 reviewed May 28, 2016
View reviewed changes

Fishrock123 reviewed Jun 3, 2016
View reviewed changes

jasnell mentioned this pull request Jun 6, 2016
doc: more detail on when timers are executed #7166
Closed
2 tasks

bengl mentioned this pull request Jun 6, 2016
doc: add timer classes #6206
Closed
2 tasks

Fishrock123 removed their assignment Jun 15, 2016

jasnell force-pushed the doc-timers-copy branch from b707c46 to 5036973Compare June 21, 2016 17:30

Fishrock123 reviewed Jun 27, 2016
View reviewed changes

Comment threaddoc/api/globals.md Outdated
Fishrock123Jun 27, 2016
Copy link
Copy Markdown
Contributor
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?

jasnell force-pushed the doc-timers-copy branch from 5036973 to fb05064Compare June 27, 2016 16:04

jasnell force-pushed the doc-timers-copy branch from fb05064 to 4091f25Compare June 28, 2016 14:28

jasnell closed this Jun 28, 2016

Fishrock123 mentioned this pull request Jul 5, 2016
Propose v6.3.0 (v2) #7550
Merged

MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed

MylesBorins added the dont-land-on-v4.x label Jul 11, 2016

blacksun1 mentioned this pull request Jan 17, 2017
observed setImmediate and setTimeout execution order contradicts documentation #7145
Closed

Uh oh!

Uh oh!

Conversation

jasnell commented May 23, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eljefedelrodeodeljefe commented May 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented May 24, 2016

Uh oh!

Fishrock123May 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented May 24, 2016

Uh oh!

jasnell commented May 24, 2016

Uh oh!

bengl commented May 27, 2016

Uh oh!

jasnell commented May 27, 2016

Uh oh!

bengl commented May 27, 2016

Uh oh!

jasnell commented May 28, 2016

Uh oh!

jasnell commented May 28, 2016

Uh oh!

bengl commented May 28, 2016

Uh oh!

benjamingr commented May 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 2, 2016

Uh oh!

jasnell commented Jun 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jun 3, 2016

Uh oh!

Fishrock123 commented Jun 15, 2016

Uh oh!

jasnell commented Jun 15, 2016

Uh oh!

jasnell commented Jun 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jun 27, 2016

Uh oh!

jasnell commented Jun 27, 2016

Uh oh!

Fishrock123 commented Jun 28, 2016

Uh oh!

jasnell commented Jun 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Fishrock123May 24, 2016•
edited
Loading