src,permission: emit dc messages from C++ and use --permission-audit #61869
src,permission: emit dc messages from C++ and use --permission-audit #61869RafaelGSS wants to merge 6 commits intonodejs:mainfrom
Conversation
Add a C++ API for diagnostics channels that allows native code to check for subscribers and publish messages without unnecessary JS boundary crossings. Uses a shared AliasedUint32Array buffer between C++ and JS to track subscriber counts per channel, enabling a fast inline check (HasSubscribers) that reads the buffer directly.
Add --permission-audit flag that enables the permission model in warning-only mode. Instead of throwing ERR_ACCESS_DENIED, it emits a message via diagnostics channel and allows the operation to continue. Publish permission check results to per-scope diagnostics channels (e.g., node:permission-model:fs) so users can observe permission decisions at runtime via diagnostics_channel. Refs: nodejs#59935
|
Review requested:
|
Qard
left a comment
There was a problem hiding this comment.
Very cool! Not going to block on it, but it would be ideal if we could have a more direct ObjectWrap pattern to link native channels to their JS counterpart and do publishes more directly rather than through this lookup table and callback doing a channel lookup on every publish.
One thing that may need some additional thought either way is what to do if a publish call happens at a point where it's not valid to call into JS at that exact time. What do we do in that situation? Panic? Schedule for next tick?
There was a problem hiding this comment.
Not a huge fan of needing to call dc.channel(...) within the publish hot-path.
Was there some reason you didn't go for just having the C++ Channel type use ObjectWrap::Wrap(...) to wrap the actual channel object itself? I know we might not be able to do so immediately as the C++ channels might be initialized before we can actually get into JS, but we can late-bind, right?
Perhaps rather than a setPublishCallback we could have something like:
dc_binding.linkNativeChannel((name) => {
return dc.channel(name);
});When that is first called it'd immediately wrap all not-yet-wrapped native channels around their JS equivalent and then store the callback to do so for any future native channels.
There was a problem hiding this comment.
I guess we could check if there are pending when linkNativeChannel is called. Pushing a commit shortly.
The reason I didn't use the ObjectWrap was due to a much larger refactor that would be necessary on diagnostics_channel js, and I was afraid of breaking/slowing the current status quo.
There was a problem hiding this comment.
What about it would need refactoring? As long as the C++ side only covers publishing into JS it should only ever need to wrap a C++ type around the JS type when the C++ actually wants to publish to it. We can just wrap lazily, only doing so when an actual C++ channel interface is requested for a particular channel name. We can negotiate that through the linkNativeChannel by only calling that for channels that need to locate their JS counterpart.
If we want deeper integration in the future we can always change that to share the channel map with the C++ side in the future, but I'm not sure if we'll actually need that unless someone has a use case for the native side subscribing to the JS side, but then we have some more complicated things to figure out anyway, like message serialization.
|
cc @bengl You may also have opinions on the diagnostics_channel implementation in here. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #61869 +/- ##
==========================================
- Coverage 89.76% 89.71% -0.05%
==========================================
Files 675 677 +2
Lines 204674 204990 +316
Branches 39330 39378 +48
==========================================
+ Hits 183716 183904 +188
- Misses 13235 13301 +66
- Partials 7723 7785 +62
🚀 New features to boost your workflow:
|
This PR supersedes #60578.
src,permission: add --permission-audit
src: add C++ support for diagnostics channels
PTAL @joyeecheung @Qard