fix(jsonrpc): cancel locally first and still emit $/cancelRequest once#909
fix(jsonrpc): cancel locally first and still emit $/cancelRequest once#909sebthom wants to merge 1 commit into
Conversation
sebthom
commented
Oct 31, 2025
- Prevent a race where a server's RequestCancelled response completes the future with ResponseErrorException before local cancellation, breaking expected CancellationException behavior.
- Change cancel(boolean) to cancel the future locally first, then (root-only) send $/cancelRequest.
- Adjust cancelRequest() to emit once even if the future is already completed locally by removing the isDone() short-circuit; dedup remains via cancelSent.
- Prevent a race where a server's RequestCancelled response completes the future with ResponseErrorException before local cancellation, breaking expected CancellationException behavior. - Change cancel(boolean) to cancel the future locally first, then (root-only) send $/cancelRequest. - Adjust cancelRequest() to emit once even if the future is already completed locally by removing the isDone() short-circuit; dedup remains via cancelSent.
|
@sebthom As mentioned in #907 (comment), I don't think that we should try to fix what is not broken. Therefore, I propose to close this PR in favour of #910, if you don't mind. Thanks again for looking into this issue, it is much appreciated! |
|
I think it still makes sense to flip the logic and cancel the local future first. but it's up to you. |
|
I think that it can be implemented either way, but it does not matter much in a multi-threaded environment. Even if we flip the logic and cancel the future first, thread B can call Just to reiterate, attempting to cancel a future is inherently racy with regard to the future completion. Clients need to check the return value of |