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: always set _destroyed #29595

Closed
wants to merge 1 commit into from

Conversation

Fishrock123
Copy link
Contributor

Required for other potential changes.

This should make it so we can always just check _destroyed to check if a timer has been ended.

I think this was included in some other PR previously, perhaps it wasn't merged or something.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Fishrock123 Fishrock123 added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 17, 2019
test/parallel/test-timers-destroyed.js Outdated Show resolved Hide resolved
@apapirovski
Copy link
Member

You’re probably thinking of https://github.com/nodejs/node/pull/27345/files. I’m still intending to merge. Personal things have been im the way.

@Fishrock123
Copy link
Contributor Author

Yeah, that one.

@apapirovski Would you be ok with this landing and then that rebasing once you have time?

@apapirovski
Copy link
Member

@Fishrock123 yup, totally!

Required for other potential changes.

This should make it so we can always just check _destroyed to
check if a timer has been ended.
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Longer term, if this is going to continue being an API accessible to the public, we should put it behind a non _'prefixed property.

Copy link

@patilharshal16 patilharshal16 left a comment

Choose a reason for hiding this comment

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

looks good

@Fishrock123 Fishrock123 removed the request for review from apapirovski September 22, 2019 18:14
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 22, 2019

The lint failure was my mistake and was force-pushed out. I'll kick off CI here again. Sorry.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 22, 2019

Landed in a861b84

@Trott Trott closed this Sep 22, 2019
Trott pushed a commit that referenced this pull request Sep 22, 2019
Required for other potential changes.

This should make it so we can always just check _destroyed to
check if a timer has been ended.

PR-URL: #29595
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Fishrock123
Copy link
Contributor Author

Longer term, if this is going to continue being an API accessible to the public, we should put it behind a non _'prefixed property.

Sorry, I forgot to address this. I don't recall exactly why this wasn't just a Symbol but this property isn't particularly intended to be public. This change exists to facilitate other core functionality / cleanup.

targos pushed a commit that referenced this pull request Sep 23, 2019
Required for other potential changes.

This should make it so we can always just check _destroyed to
check if a timer has been ended.

PR-URL: #29595
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
Required for other potential changes.

This should make it so we can always just check _destroyed to
check if a timer has been ended.

PR-URL: #29595
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants