Skip to content

Conversation

@sebthom
Copy link
Contributor

@sebthom 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.
@pisv
Copy link
Contributor

pisv commented Nov 2, 2025

@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!

@sebthom
Copy link
Contributor Author

sebthom commented Nov 2, 2025

I think it still makes sense to flip the logic and cancel the local future first. but it's up to you.

@pisv
Copy link
Contributor

pisv commented Nov 2, 2025

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 cancelRequest while thread A is calling super.cancel, and the request can complete with ResponseErrorCode.RequestCancelled before thread A gets to cancel the future.

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 future.cancel if necessary and should not make any assumptions about implementation details.

@sebthom sebthom closed this Nov 3, 2025
@sebthom sebthom deleted the JsonRpcRequestFuture2 branch November 9, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants