GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

GeezerVison is showing github.com — diagnostics_channel: fix ref counting bug when reaching zero subscribers by Qard · Pull Request #47520 · nodejs/node

Conversation

Qard commented Apr 12, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction

Qard added confirmed-bug

Issues and PRs for confirmed bugs.

diagnostics_channel Issues and PRs related to the diagnostics_channel module. labels

nodejs-github-bot added the needs-ci PRs that need a full CI run. label

simon-id reviewed

View reviewed changes

simon-id left a comment

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Qard commented Apr 12, 2023

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tlhunter reviewed

View reviewed changes

Comment thread

test/parallel/test-diagnostics-channel-pub-sub.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

rochdev reviewed

View reviewed changes

Comment thread

lib/diagnostics_channel.js Resolved

Uh oh!

There was an error while loading. Please reload this page.


          diagnostics_channel: fix ref counting bug when reaching zero subscribers

2fcddb3

Qard force-pushed the fix-diagnostics-channel-ref-counting branch from b749f53 to 2fcddb3 Compare

April 12, 2023 16:13

Qard added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label

github-actions Bot commented Apr 12, 2023

Copy link

Copy Markdown

Contributor

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

bricss, RafaelGSS, ronag, and Flarna reacted with thumbs up emoji

All reactions

👍 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Qard mentioned this pull request

2023-04-18, Version 20.0.0 (Current) #47381

Merged

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

This comment was marked as outdated.

Sign in to view

ronag approved these changes

View reviewed changes

ronag left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

RafaelGSS approved these changes

View reviewed changes

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

This comment was marked as outdated.

Sign in to view

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

This comment was marked as outdated.

Sign in to view

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

nodejs-github-bot commented Apr 12, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Flarna approved these changes

View reviewed changes

rochdev mentioned this pull request

switch to internal diagnostics channel with node bug patch DataDog/dd-trace-js#3002

Merged

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

This comment was marked as outdated.

Sign in to view

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

Qard added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Apr 13, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

RafaelGSS added the commit-queue PRs queued for automated landing through the Commit Queue. label

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

nodejs-github-bot merged commit 6fb74c7 into nodejs:main

nodejs-github-bot commented Apr 13, 2023

Copy link

Copy Markdown

Collaborator

Landed in 6fb74c7

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Qard deleted the fix-diagnostics-channel-ref-counting branch

April 13, 2023 18:49

RafaelGSS pushed a commit that referenced this pull request


          diagnostics_channel: fix ref counting bug when reaching zero subscribers

c02a7e7

PR-URL: #47520
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>

github-actions Bot mentioned this pull request

CI Reliability 2023-04-15 nodejs/reliability#543

Open

33 tasks

danielleadams added the backport-requested-v18.x label

targos pushed a commit that referenced this pull request


          diagnostics_channel: fix ref counting bug when reaching zero subscribers

88f9ebb

PR-URL: #47520
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>

targos mentioned this pull request

v18.19.0 release proposal #50953

Merged

sercher added a commit to sercher/graaljs that referenced this pull request


          diagnostics_channel: fix ref counting bug when reaching zero

5b0d5cb

PR-URL: nodejs/node#47520
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>

sercher added a commit to sercher/graaljs that referenced this pull request


          diagnostics_channel: fix ref counting bug when reaching zero

bf2db5e

PR-URL: nodejs/node#47520
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>

secondfry mentioned this pull request

feat!: use modern diagnostics_channel API open-telemetry/opentelemetry-js-contrib#2407

Closed

This was referenced May 8, 2026

repro: ws-stress fails on master via nyc-cache + dc-polyfill channel re-creation DataDog/dd-trace-js#8297

Closed

fix: memoize channel(name) to make Channel identity stable per name DataDog/dc-polyfill#27

Merged

fix: memoize tracingChannel(name) to stabilize sub-channel identity DataDog/dc-polyfill#28

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ronag approved these changes

Flarna approved these changes

RafaelGSS approved these changes

+3 more reviewers

tlhunter left review comments

rochdev left review comments

simon-id left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Issues and PRs for confirmed bugs.

diagnostics_channel

Issues and PRs related to the diagnostics_channel module.

PRs proposed for a shorter-than-standard waiting period before landing.

PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.