diagnostics_channel: add BoundedChannel and scopes#61680
diagnostics_channel: add BoundedChannel and scopes#61680Qard wants to merge 3 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/21678728231 |
c764f06 to
1018c57
Compare
RafaelGSS
left a comment
There was a problem hiding this comment.
Could you please expand on PR description on why this is needed? We have channels, tracingChannels and now windowChannel, it would be great to know for which kind of situations we need each one of them.
| added: REPLACEME | ||
| --> | ||
|
|
||
| > Stability: 1 - Experimental |
There was a problem hiding this comment.
| > Stability: 1 - Experimental | |
| > Stability: 1 - Experimental |
Perhaps 1.1 Active Development instead?
There was a problem hiding this comment.
The particular change is inherited from #61674. Do you want that made there? What about the new APIs in diagnostics_channel? Same status?
|
It's also replacing most of the internals of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61680 +/- ##
========================================
Coverage 89.68% 89.69%
========================================
Files 676 677 +1
Lines 206578 206784 +206
Branches 39555 39588 +33
========================================
+ Hits 185267 185470 +203
- Misses 13446 13448 +2
- Partials 7865 7866 +1
🚀 New features to boost your workflow:
|
doc/api/diagnostics_channel.md
Outdated
| }); | ||
| ``` | ||
|
|
||
| #### `diagnostics_channel.windowChannel(nameOrChannels)` |
There was a problem hiding this comment.
This name window channel is confusing. It's not related to the specific window API in JavaScript. It reads to me that it is a version of tracing channel that enables using support. I feel terms like scope could be a more intuitive name.
There was a problem hiding this comment.
I feel "scope" is an equally problematic name in that it makes many think of lexical scope which is typically associated with closures, which is exactly what this is explicitly trying to not be. Naming is hard. 🤷🏻
I'm happy to pick a different name if you have a better idea for one, but I'm not convinced "scope" is much better. I had also considered "block" but that also has weird associations with "blocking" which is again misleading. 😐
There was a problem hiding this comment.
I went with BoundedChannel. Hopefully that is clear and unique enough naming.
Adds support for using scope = storage.withScope(data) to do the equivalent of a storage.run(data, fn) with using syntax. This enables avoiding unnecessary closures.
This adds
BoundedChannel, adds ausingsyntax equivalent torunStores, and modifies the internals ofTracingChannelto use these to avoid closures in several places.Why BoundedChannel?
Reviewers asked why a new class is needed instead of just using the existing
channel()ortracingChannel()APIs. Here's the motivation:vs
channel(): A bareChannelhas no built-in concept of start/end lifecycle events or store binding for async context propagation.BoundedChannelcombines two channels with awithScope()/run()API that tiesstart→ store entry → user code →end→ store exit into a single unit, which a rawchannelcan't express.vs
tracingChannel():TracingChannelis designed for tracing operations with async continuations (it has 5 events:start,end,asyncStart,asyncEnd,error).BoundedChannelis a simpler, synchronous-only primitive with juststartandend. It is also used internally byTracingChannelto implement its call window and continuation window, making the code cleaner and removing several closure allocations.The name "Bounded" reflects that the scope is bounded by the
usingblock — start fires when the scope begins, end fires when it is disposed.Depends on #61674
cc @nodejs/diagnostics