-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
diagnostics_channel: fix ref counting bug when reaching zero subscribers #47520
diagnostics_channel: fix ref counting bug when reaching zero subscribers #47520
Conversation
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.
lint issues otherwise LGTM
Yep, I'll fix those in the morning. My editor always seems to want to inject those spaces where they shouldn't be. 🙈 |
b749f53
to
2fcddb3
Compare
Fast-track has been requested by @Qard. Please 👍 to approve. |
This comment was marked as outdated.
This comment was marked as outdated.
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.
RSLGTM
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Landed in 6fb74c7 |
PR-URL: #47520 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
PR-URL: #47520 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
PR-URL: #47520 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
PR-URL: #47520 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
PR-URL: nodejs/node#47520 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
PR-URL: nodejs/node#47520 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
I discovered a bug in some of the changes I landed with TracingChannel. Technically the bug existed before, but we've been discouraging use of
channel.subscribe(...)
andchannel.unsubscribe(...)
. With the TracingChannel changes I included a fix for the GC issue that allowschannel.subscribe(...)
andchannel.unsubscribe(...)
to work as they were originally intended, and some simplifications were made to make the top-level functions delegate to those. Unfortunately, there was an unnoticed issue where if the subscriber count ever dropped to zero the weakref would be deleted from the channels map but channel objects could still be held and therefore could still have subscribers added which would attempt an incRef on a weakref that's not there anymore. To handle that safely the delete actually has to happen only when there are no more strong refs, meaning when GC happens, so I've changed it to useFinalizationRegistry
instead.