worker: emit worker exit notifications on BroadcastChannel - #65575
SudhansuBandha wants to merge 3 commits into
Conversation
Labeeb2339
left a comment
There was a problem hiding this comment.
The current lint-cpp check reports 10 style errors in the new C++ code, so this PR is not yet lint-clean. Notably: trailing whitespace at src/node_messaging.cc:855 and src/node_messaging.h:194/233/235, an over-80-character line at src/node_messaging.cc:896, missing spaces in if(environment->is_stopping()) at :970, and incorrect indentation around WorkerExitNotification in src/node_messaging.h:226-229. Running the repository C++ formatter/linter and committing its output should address these before the functional review proceeds.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65575 +/- ##
==========================================
+ Coverage 90.05% 90.07% +0.02%
==========================================
Files 751 751
Lines 254868 254944 +76
Branches 48107 48122 +15
==========================================
+ Hits 229531 229653 +122
+ Misses 16511 16471 -40
+ Partials 8826 8820 -6
🚀 New features to boost your workflow:
|
4dd297f to
d8a79bf
Compare
| * Type: {Function} Invoked with a received message cannot be | ||
| deserialized. | ||
|
|
||
| ### `broadcastChannel.onworkerexited` |
There was a problem hiding this comment.
This needs a YAML tag for version history tracking
There was a problem hiding this comment.
Updated the fix in latest commit
|
|
||
| if (worker_exit_notifications_.empty()) return false; | ||
|
|
||
| *notification = worker_exit_notifications_.front(); |
There was a problem hiding this comment.
Prefer std::optional<> instead of assigning to out parameters (esp. if they have non-trivial types)
There was a problem hiding this comment.
Updated the fix in latest commit
| return; | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is duplicating a significant amount of logic – is there a reason that this needs to be a new message type, and cannot be something that would be conveyed through normal messages in the queue?
There was a problem hiding this comment.
I tried to implement a separate pipeline since this notification is related to worker lifecycle instead of inter thread messages of Broadcast Channel. If this is not something to be done then I will update the PR to use existing pipeline for emitting these lifecycle events.
There was a problem hiding this comment.
I think that would significantly simplify the implementation and make it easier to write consumers -- events will happen in order from their perspective, rather than seeing a worker exit before all its messages have been sent
There was a problem hiding this comment.
Sure, I will implement the suggested changes. Thanks for your feedback!!
There was a problem hiding this comment.
Updated the implementation to use existing queue for these events
| const ExitCode exit_code = environment->exit_code(ExitCode::kNoFailure); | ||
|
|
||
| Debug(this, | ||
| "Worker exiting: thread_id=%" PRIu64 ", exit_code=%d", |
There was a problem hiding this comment.
| "Worker exiting: thread_id=%" PRIu64 ", exit_code=%d", | |
| "Worker exiting: thread_id=%d, exit_code=%d", |
Debug() doesn't care about the actual type anyway
There was a problem hiding this comment.
Updated the fix in latest commit
f23efdb to
b1c9fb9
Compare
|
The GitHub Actions CI workflow had previously failed due to problems with GitHub. I triggered re-runs, however there are now other failures. The PR branch is now more that 300 commits behind the Please rebase the branch in this PR according to the Pull requests documentation to make sure that all available fixes are included. |
Expose worker termination notifications through BroadcastChannel so consumers can observe when a worker exits andinspect its thread ID and exit code. Fixes: nodejs#59053 Signed-off-by: SudhansuBandha <bandhasudhansu@gmail.com>
b1c9fb9 to
f3007df
Compare
|
GitHub Actions CI workflow tests are hanging up and will probably time out after the default 6 hour maximum duration is reached. It does not look like other current PRs have this issue. See Pull requests > Step 6: Test
with further details under BUILDING > Running tests. |
Expose worker termination notifications through BroadcastChannel so consumers can observe when a worker exits and inspect its thread ID and exit code.
Fixes: #59053