Skip to content

timers: always set _destroyed#29595

Closed
Fishrock123 wants to merge 1 commit intonodejs:masterfrom
Fishrock123:timers-destroyed
Closed

timers: always set _destroyed#29595
Fishrock123 wants to merge 1 commit intonodejs:masterfrom
Fishrock123:timers-destroyed

Conversation

@Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Sep 17, 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.

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
addaleax
addaleax approved these changes Sep 17, 2019
View reviewed changes
@apapirovski
Copy link
Contributor

apapirovski commented Sep 18, 2019

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

Fishrock123 commented Sep 18, 2019

Yeah, that one.

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

@apapirovski
Copy link
Contributor

apapirovski commented Sep 18, 2019

@Fishrock123 yup, totally!

@Fishrock123
timers: always set _destroyed
57e80a7 
Required for other potential changes.

This should make it so we can always just check _destroyed to
check if a timer has been ended.
jasnell
jasnell approved these changes Sep 18, 2019
View reviewed changes
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.

patilharshal16
patilharshal16 reviewed Sep 19, 2019
View reviewed changes
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

nodejs-github-bot commented Sep 22, 2019

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

@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

nodejs-github-bot commented Sep 22, 2019

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

@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
@Fishrock123 @Trott
timers: set _destroyed even if there are no destroy-hooks
a861b84 
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

Fishrock123 commented Sep 22, 2019

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
@Fishrock123 @targos
timers: set _destroyed even if there are no destroy-hooks
16b231d 
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
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@Fishrock123 @BridgeAR
timers: set _destroyed even if there are no destroy-hooks
b43d2dd 
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>
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

+1 more reviewer

@patilharshal16 patilharshal16 patilharshal16 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Fishrock123 @apapirovski @nodejs-github-bot @Trott @jasnell @addaleax @lpinca @patilharshal16 @BridgeAR