fix: make sure register handler when ipc emitter add listener - #323663
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes ChannelClient.requestEvent() in src/vs/base/parts/ipc/common/ipc.ts so that re-adding a listener to an IPC event after disposing the previous listener will correctly receive messages again.
Changes:
- Register the IPC response handler in
Emitter’sonWillAddFirstListenerhook (instead of at event creation time). - Ensure the handler is re-registered whenever the event transitions from 0 → 1 listeners, matching the existing cleanup on last-listener removal.
roblourens
requested changes
Jul 2, 2026
roblourens
left a comment
Member
There was a problem hiding this comment.
Thanks for the PR, I think this change is good but please file an issue with a better description of the issue, and add unit tests for the scenario
When the last listener is disposed, we will remove the handler from the map.
However, we will not add the handler back to the map when re-subscribing to the emitter,
so the event will not be emitted anymore. It is better register handler in onWillAddFirstListener and
delete it in onDidRemoveLastListener.
theanarkh (theanarkh)
force-pushed
the
optimize_ipc
branch
from
July 4, 2026 17:02
2ea2098 to
4f1cf3b
Compare
Contributor
Author
|
roblourens Thanks ! I have updated the code, please help review again. |
Member
|
Thanks, can you open an issue? |
Contributor
Author
Done. |
roblourens
approved these changes
Jul 7, 2026
roblourens
enabled auto-merge (squash)
July 7, 2026 14:45
Vritant Bhardwaj (vritant24)
approved these changes
Jul 7, 2026
Ben Villalobos (benvillalobos)
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fixes: #324678
When the last listener is disposed, we will remove the handler from the map. However, we will not add the handler back to the map when re-subscribing to the emitter, so the event will not be emitted anymore. It is better register handler in
onWillAddFirstListenerand delete it inonDidRemoveLastListener.example.