Skip to content

Conversation

@inonwir
Copy link

@inonwir inonwir commented Jul 7, 2025

We are working on a research project for improving open-source projects, by using the latest accepted answer from Stack Overflow that matched with your code snippet. We found this recommendation for improving your code from https://stackoverflow.com/questions/2180534.

Note: Our study is approved by the Institutional Review Board of Mahidol University. You can find the participant information sheet explaining this study HERE.

Type of Recommendation: Add Null Check
Reason: Prevent NullPointerException
Type: Bug
Influences: Fix method bytesToHexString (edited)
[10:42]

Type of Recommendation: Add safe cancellation support to FutureCallback
Reason: Prevent hanging get() calls and ensure compliance with Future API
Type: Bug Fix
Influences: Implements cancel(), updates get() to throw CancellationException, and adds proper isCancelled() state handling

Add safe cancellation support to FutureCallback
This PR adds proper cancellation support to `FutureCallback`:
- Implements `cancel()` to mark the future as cancelled and unblock `get()`
- `get()` now throws `CancellationException` if cancelled
- `isCancelled()` reflects the state

This makes `FutureCallback` fully compliant with the `Future` API and prevents hanging `get()` calls.

No breaking changes.
@zackthehuman
Copy link
Contributor

@inonwir Thanks for the contribution. I have some questions about why this change is necessary. If you look at the implementation of FutureCallback, it was intentionally designed to not support cancellation. Since it is an adapter that makes a Callback work as a Future, and Callback doesn't have any kind of cancellation semantics, neither would a FutureCallback.

What does it mean to "cancel" a callback? It doesn't make sense.

@inonwir
Copy link
Author

inonwir commented Aug 19, 2025

@zackthehuman
Thanks for pointing this out! You’re right — this doesn’t cancel the underlying Callback work. The change only lets callers cancel their interest in the Future: cancel() marks the Future as cancelled, unblocks any waiting threads, and get() throws CancellationException. This avoids hanging get() calls and keeps FutureCallback consistent with the standard Future API, without breaking existing behavior.

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