switch to internal diagnostics channel with node bug patch #3002
Conversation
Overall package sizeSelf size: 4.03 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report
@@ Coverage Diff @@
## master #3002 +/- ##
==========================================
- Coverage 87.65% 87.63% -0.02%
==========================================
Files 329 329
Lines 11753 11762 +9
Branches 33 33
==========================================
+ Hits 10302 10308 +6
- Misses 1451 1454 +3
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
BenchmarksComparing candidate commit 6fd28e0 in PR branch Found 0 performance improvements and 10 performance regressions! Performance is the same for 660 metrics, 38 unstable metrics. scenario:startup-control-everything-14
scenario:startup-with-tracer-everything-14
scenario:startup-control-everything-16
scenario:startup-with-tracer-everything-16
scenario:startup-control-everything-18
scenario:startup-with-tracer-everything-18
|
|
For some reason it seems "everything" benchmarks include dev dependencies which is why they run slower. |
|
|
||
| this.subscribe(() => {}) // Keep it active forever. | ||
| } | ||
| } |
There was a problem hiding this comment.
If it's already active, we still need to be sure there's an extra subscriber forever.
| } | |
| } else { | |
| maybeInactive.subscribe(() => {}) // Keep it active forever. | |
| } |
There was a problem hiding this comment.
How could it be active? It would have to first be inactive before it's ever active.
There was a problem hiding this comment.
All tests passing also prove that this is the case, and preventing usage of the built-in with eslint ensures that nothing subscribes with the built-in directly.
There was a problem hiding this comment.
If something else subscribes to that same named channel before we get to this point it will already be active. Unlikely given we're currently using our own custom channel names, but possible. And it will become more likely as we start pushing instrumentations upstream.
There was a problem hiding this comment.
I changed the approach to support active channels safely, let me know if that works.
There was a problem hiding this comment.
If you're going to use the WeakMap, why even patch the subscribe function? You could just immediate do this.subscribe(() => {}) on any channel not already seen. Also, the return value of the patched subscribe is wrong, it should return the result of the subscribe call.
There was a problem hiding this comment.
If you're going to use the WeakMap, why even patch the subscribe function?
Because otherwise a subscriber would be added on channel() and not on subscribe(), so publishers would cause a subscription as well.
Also, the return value of the patched subscribe is wrong, it should return the result of the subscribe call.
True, I didn't return anything because the method doesn't either, but it's best practice to return anyway in case that changes.
There was a problem hiding this comment.
Ah, right, it's only unsubscribe that returns a value. Misremembering my own code. 😆
What does this PR do?
Switch to internal diagnostics channel with Node bug patch.
Motivation
Avoid issues mentioned in nodejs/node#47520
Additional Notes
We don't really have a good strategy to test issues that only happen in very specific version of Node, so not sure how to test this other than with 19.9.0 being the current version and also the version that is broken.