doc: add active, enroll and unenroll #11736

jsumners · 2017-03-07T23:47:26Z

This PR adds documentation for the public timers methods
active, enroll and unenroll.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

mscdex · 2017-03-07T23:58:13Z

doc/api/timers.md

+Example:
+
+```js
+var timers = require('timers')


This and other lines are missing semicolons.

mscdex · 2017-03-07T23:58:43Z

doc/api/timers.md

+```js
+var timers = require('timers')
+var atimer = {
+  _onTimeout: function () {


minor nit: should be function() {

mscdex · 2017-03-08T00:00:12Z

I'm not sure if these methods are actually intended to be public or not, even though they are not underscore-prefixed?

/cc @Fishrock123

This commit adds documentation for the public `timers` methods `active`, `enroll` and `unenroll`.

jasnell · 2017-03-08T01:49:49Z

At this point (a) they're public and (b) being used, we may as well document them.

Fishrock123 · 2017-03-08T15:09:25Z

I very much don't think we should document them as for public use.

The memory use is negligible and is far far less difference than "slightly".
You need to pre-setup objects to prevent them from de-opting (hidden class changes).
The API is very confusing.

It may be worthwhile to mention more specifics of the timers implementation that relate to these, such as that unenroll() is called at pretty much every cancel path, among other minor details.

If we want to mention actually useful stuff, then we should probably mention just active(), which can be used to refresh a timeout timer without creating a new object.

jsumners · 2017-03-08T15:14:28Z

I couldn't think of much for an introduction. So I copied the point made about enroll that is in the source code and added my observation about more control. If there are suggestions for a better introduction, I am more than willing to listen.

As for the API being confusing, it was more confusing when I learned about it via its usage in https://github.com/davidmarkclements/fast-date/blob/master/fallback.js and there wasn't any documentation on it.

Fishrock123 · 2017-03-08T15:24:16Z

As for the API being confusing, it was more confusing when I learned about it via its usage in https://github.com/davidmarkclements/fast-date/blob/master/fallback.js and there wasn't any documentation on it.

uuuugh ok. Let's document it but tell people to not use it / avoid it.

jsumners · 2017-03-08T17:48:42Z

@Fishrock123 how would you revise the introduction?

jasnell · 2017-03-08T18:44:44Z

Aside: I've been stewing over whether we need an official not-recommended-for-use-but-not-quite-deprecated status for various APIs or if calling them out individually as don't use it on a case by case basis is enough. The challenge is that people will flat out ignore it but having a clearly documented status gives us some leeway for making changes later if necessary.

jsumners · 2017-03-08T19:27:00Z

@jasnell would the classification you're looking for be "volatile"?

jasnell · 2017-03-08T20:13:34Z

No, because the APIs in question might not change frequently or at all. They may ... the defining characteristic would be that they simply are not recommended (for whatever reason)

Qard · 2017-03-08T20:40:25Z

In a way, I think it's similar to unsafe in Rust. You could enroll an object and forget to use it, leaking memory.

joyeecheung · 2017-03-09T10:37:15Z

@jasnell Maybe make those APIs non-enumerable so they don't show up in auto-completion would be a start..

jasnell · 2017-03-15T00:32:48Z

@nodejs/ctc @Fishrock123 ... would appreciate more input on this

bnoordhuis · 2017-03-20T11:51:59Z

They were never intended for public use, just an implementation detail that leaked out due to lack of foresight.

If the argument for documenting them is to avoid confusion for people stumbling upon them, it should be a one-liner like "Implementation detail. Do not use."

davidmarkclements · 2017-03-20T14:40:05Z

Personally I'd prefer a warning about cleaning up state than warning not to use - especially because I've now used it in a published module as a performance enhancement.

jsumners · 2017-03-20T15:03:50Z

How about this intro:

It is possible to make any object a timer. The advantage of doing so is being able to
control when the timer is started, as opposed to the other timers which start immediately.

Warning: using this API means you must keep track of the timer state. If you create
a timer, but never unenroll it, then you will introduce a memory leak.

jasnell · 2017-03-20T16:49:01Z

In general, please avoid the use of directed pronouns like you or your in documentation text. The warning could be reworded as:

*Note*: When using this API, it is important to track timer enrollment state. A memory leak
will occur if a timer is enrolled but never unenrolled.

jasnell · 2017-03-20T16:51:46Z

I definitely agree with @bnoordhuis' sentiment that the added documentation should include a strong warning against using these methods. It should likely also be noted that the specific implementation details of these particular methods could change and that while the API is publicly accessible, ongoing support is not guaranteed. (That said, we would definitely have to take any changes to these through a proper deprecation cycle in practice)

jsumners · 2017-03-20T16:57:45Z

@jasnell so do you have any other changes for the proposed introduction rewording?

jasnell

In general this definitely needs the warning text and a clear description of the state management requirements.

jasnell · 2017-03-20T17:06:31Z

doc/api/timers.md

+## Manual Timers
+
+It is possible to make any object a timer. Reusing an existing object as a timer
+slightly reduces memory consumption. Another advantage is that you control


Again, please avoid the use of you in the documentation text :-)

jasnell · 2017-03-20T17:06:41Z

doc/api/timers.md

+
+```js
+var timers = require('timers');
+var atimer = {


s/var/const

jasnell · 2017-03-20T17:07:31Z

doc/api/timers.md

@@ -162,6 +162,50 @@ added: v0.0.1

 Cancels a `Timeout` object created by [`setTimeout()`][].

+## Manual Timers
+
+It is possible to make any object a timer. Reusing an existing object as a timer


I would say something like, It is possible, but not recommended, to make any object into a timer.

bnoordhuis · 2017-03-20T17:59:54Z

I'm still -1 on anything beyond a "do not use." The language proposed in #11736 (comment) is still an encouragement to use them.

It's easy enough to make a queue or linked list that hangs off a setTimeout() timer, you don't need enroll/unenroll for that.

jasnell · 2017-03-20T18:01:48Z

@bnoordhuis ... if the APIs really should not be used then we should move the implementation to lib/internal and do a runtime deprecation of the public methods.

jsumners · 2017-03-20T18:05:15Z

I agree. I understand that it may have been a mistake, but they are written with the convention implying they are a public API. My opinion is that any such API should be documented since people will find it, and they will use it under the assumption that it is public.

bnoordhuis · 2017-03-20T18:15:39Z

Then a note in the documentation explaining they shouldn't be used should suffice. I'm not out to break existing code but new code should be steered away from using them.

thefourtheye · 2017-03-20T18:40:32Z

doc/api/timers.md

+Example:
+
+```js
+var timers = require('timers');


Please encourage Strict mode.

thefourtheye · 2017-03-20T18:41:13Z

doc/api/timers.md

+  _onTimeout: function() {
+    console.log('timeout');
+  }
+}


Our coding standard expects a semi-colon at the end.

mhdawson · 2017-04-05T18:53:53Z

I agree with @bnoordhuis, given that we don't want people to use them, lets just say that and then start the deprecation process.

vsemozhetbyt · 2017-04-25T00:32:51Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/8535/

jasnell · 2017-04-25T18:13:33Z

doc/api/timers.md

+circumstances this can be advantageous. However, in general practice, using
+this API is strongly discouraged. This is due to the fact that when using this API
+it is important to track timer enrollment state. A memory leak will occur if a
+timer is enrolled but never unenrolled.


This paragraph can be simplified a bit...

It is possible (but not recommended) to make any object a timer. In general practice, however, using this API is **strongly discouraged** due to the potential for memory leaks should such timers fail to be properly unenrolled.

jasnell · 2017-04-25T18:14:46Z

doc/api/timers.md

+
+timers.enroll(atimer, 1000); // make the `atimer` object a timer
+timers.active(atimer); // start the timer
+```


This should include a clear example showing the timer being unenroll()'d per the warning above

It does show it. Line 183, in the callback from the timer completion.

BridgeAR · 2017-08-26T08:38:34Z

There was no progress here since a while and I fear this might not get merged as there were quite a few concerns about publishing this at all (and I share these concerns).
I also do not think that people who actually use non documented code will check if the documentation says "do not use this" or that they would care about it.
Therefore if there is no one strongly for including this I would like to go ahead and close this.

jasnell · 2017-08-26T14:20:44Z

Closing this is ok. I do think something should be done here but this pr apparently isn't it.

nodejs-github-bot added doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Mar 7, 2017

mscdex reviewed Mar 7, 2017

View reviewed changes

doc/api/timers.md Outdated

Example:

```js

var timers = require('timers')

Copy link

Contributor

mscdex Mar 7, 2017

Choose a reason for hiding this comment

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

This and other lines are missing semicolons.

mscdex reviewed Mar 7, 2017

View reviewed changes

doc: add active, enroll and unenroll

5a5aac5

This commit adds documentation for the public `timers` methods `active`, `enroll` and `unenroll`.

jsumners force-pushed the timer branch from 340e4cb to 5a5aac5 Compare March 8, 2017 00:35

jasnell added the ctc-review label Mar 15, 2017

Trott mentioned this pull request Mar 20, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-22 nodejs/CTC#82

Closed

jasnell requested changes Mar 20, 2017

View reviewed changes

thefourtheye reviewed Mar 20, 2017

View reviewed changes

Incorporate review requests

4d8c0f5

Trott mentioned this pull request Mar 26, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-29 nodejs/CTC#85

Closed

Trott mentioned this pull request Apr 3, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-05 nodejs/CTC#93

Closed

Trott mentioned this pull request Apr 9, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-12 nodejs/CTC#98

Closed

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

This was referenced Apr 17, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-19 nodejs/CTC#105

Closed

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-04-26 nodejs/CTC#111

Closed

jasnell requested changes Apr 25, 2017

View reviewed changes

Incorporate @jasnell latest requested changes

b659cb1

Trott mentioned this pull request May 1, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-05-03 nodejs/CTC#117

Closed

Fishrock123 self-assigned this May 1, 2017

Trott mentioned this pull request May 8, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-05-10 nodejs/CTC#120

Closed

bnoordhuis removed the ctc-review label May 20, 2017

Fishrock123 mentioned this pull request May 22, 2017

timers: expose rearm() #13144

Closed

benjamingr closed this Sep 1, 2017

doc: add active, enroll and unenroll #11736

doc: add active, enroll and unenroll #11736

Conversation

jsumners commented Mar 7, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Mar 8, 2017

jasnell commented Mar 8, 2017

Fishrock123 commented Mar 8, 2017 • edited Loading

jsumners commented Mar 8, 2017

Fishrock123 commented Mar 8, 2017 • edited Loading

jsumners commented Mar 8, 2017

jasnell commented Mar 8, 2017

jsumners commented Mar 8, 2017

jasnell commented Mar 8, 2017

Qard commented Mar 8, 2017

joyeecheung commented Mar 9, 2017

jasnell commented Mar 15, 2017

bnoordhuis commented Mar 20, 2017

davidmarkclements commented Mar 20, 2017

jsumners commented Mar 20, 2017

jasnell commented Mar 20, 2017

jasnell commented Mar 20, 2017

jsumners commented Mar 20, 2017

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Mar 20, 2017

jasnell commented Mar 20, 2017

jsumners commented Mar 20, 2017

bnoordhuis commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Apr 5, 2017

vsemozhetbyt commented Apr 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Aug 26, 2017

jasnell commented Aug 26, 2017

Fishrock123 commented Mar 8, 2017 •

edited

Loading

Fishrock123 commented Mar 8, 2017 •

edited

Loading