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

diagnostics_channel: fix ref counting bug when reaching zero subscribers #47520

Merged

Conversation

Qard
Copy link
Member

@Qard Qard commented Apr 12, 2023

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(...) and channel.unsubscribe(...). With the TracingChannel changes I included a fix for the GC issue that allows channel.subscribe(...) and channel.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 use FinalizationRegistry instead.

@Qard Qard added confirmed-bug Issues with confirmed bugs. diagnostics_channel Issues and PRs related to diagnostics channel labels Apr 12, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 12, 2023
Copy link
Contributor

@simon-id simon-id left a 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

@Qard
Copy link
Member Author

Qard commented Apr 12, 2023

Yep, I'll fix those in the morning. My editor always seems to want to inject those spaces where they shouldn't be. 🙈

lib/diagnostics_channel.js Outdated Show resolved Hide resolved
@Qard Qard force-pushed the fix-diagnostics-channel-ref-counting branch from b749f53 to 2fcddb3 Compare April 12, 2023 16:13
@Qard Qard added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 12, 2023
@github-actions
Copy link
Contributor

Fast-track has been requested by @Qard. Please 👍 to approve.

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

RSLGTM

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@nodejs-github-bot

This comment was marked as outdated.

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@nodejs-github-bot

This comment was marked as outdated.

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@nodejs-github-bot

This comment was marked as outdated.

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 13, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6fb74c7 into nodejs:main Apr 13, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6fb74c7

RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
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>
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
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>
@Qard Qard deleted the fix-diagnostics-channel-ref-counting branch April 13, 2023 18:49
RafaelGSS pushed a commit that referenced this pull request Apr 13, 2023
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>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 3, 2023
targos pushed a commit that referenced this pull request Nov 10, 2023
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>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. confirmed-bug Issues with confirmed bugs. diagnostics_channel Issues and PRs related to diagnostics channel fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants