Copilot commented on code in PR #7921:
URL: https://github.com/apache/ignite-3/pull/7921#discussion_r3029520524
##########
modules/api/src/main/java/org/apache/ignite/lang/CancelHandleImpl.java:
##########
@@ -120,6 +152,8 @@ void cancel() {
cancelFut = CompletableFuture.allOf(futures).whenComplete((r,
t) -> {
handle.cancelFut.complete(null);
});
+
+ listenersCopy = new ArrayList<>(listeners);
Review Comment:
Listener callbacks are copied and executed on cancel, but the backing
`listeners` list is never cleared. After cancellation, the token will retain
strong references to all registered callbacks indefinitely, which creates a
concrete memory-retention issue (including for callbacks that capture large
object graphs). Consider clearing `listeners` under `mux` after creating
`listenersCopy` (or otherwise dropping references) once cancellation is
requested.
```suggestion
listenersCopy = new ArrayList<>(listeners);
listeners.clear();
```
##########
modules/api/src/main/java/org/apache/ignite/lang/CancelHandleImpl.java:
##########
@@ -136,6 +170,18 @@ void cancel() {
}
}
+ // Run listener callbacks outside of lock
+ for (Runnable listener : listenersCopy) {
Review Comment:
Listener callbacks are copied and executed on cancel, but the backing
`listeners` list is never cleared. After cancellation, the token will retain
strong references to all registered callbacks indefinitely, which creates a
concrete memory-retention issue (including for callbacks that capture large
object graphs). Consider clearing `listeners` under `mux` after creating
`listenersCopy` (or otherwise dropping references) once cancellation is
requested.
##########
modules/core/src/main/java/org/apache/ignite/lang/CancelHandleHelper.java:
##########
@@ -78,17 +78,6 @@ public static void addCancelAction(
addCancelAction(token, () -> completionFut.cancel(true),
completionFut);
}
Review Comment:
`CancelHandleHelper.isCancelled(CancellationToken)` was removed, which can
be a breaking change for any internal/external consumers that depended on this
helper (it was `public static`). If backward compatibility is desired, consider
keeping a deprecated shim method (delegating to `token.isCancelled()`) for at
least one release cycle.
```suggestion
/**
* Returns {@code true} if the given cancellation token has been
cancelled.
*
* @deprecated Use {@link CancellationToken#isCancelled()} directly
instead.
*/
@Deprecated
public static boolean isCancelled(CancellationToken token) {
Objects.requireNonNull(token, "token");
return token.isCancelled();
}
```
##########
modules/api/src/main/java/org/apache/ignite/lang/CancelHandleImpl.java:
##########
@@ -96,12 +101,39 @@ void addCancelAction(Runnable action, CompletableFuture<?>
fut) {
}
}
- boolean isCancelled() {
+ @Override
+ public boolean isCancelled() {
return cancelFut != null;
}
+ @Override
+ public AutoCloseable listen(Runnable callback) {
+ Objects.requireNonNull(callback, "callback");
+
+ if (cancelFut != null) {
+ callback.run();
+ return () -> { };
+ }
+
+ synchronized (mux) {
+ if (cancelFut == null) {
+ listeners.add(callback);
+ return () -> {
+ synchronized (mux) {
+ listeners.remove(callback);
+ }
+ };
+ }
+ }
+
+ callback.run();
+ return () -> { };
+ }
Review Comment:
When the token is already cancelled, `listen(...)` invokes `callback.run()`
synchronously and lets any exception escape directly to the caller. When the
callback is invoked during `cancel()`, exceptions are caught and
wrapped/suppressed into an `IgniteException`. This makes listener error
behavior depend on timing. Consider defining and enforcing one consistent
error-handling contract (e.g., always catching/wrapping, or always propagating)
and aligning both paths to it; otherwise users can see different exception
types for the same callback depending on race timing.
##########
modules/api/src/main/java/org/apache/ignite/lang/CancelHandleImpl.java:
##########
@@ -96,12 +101,39 @@ void addCancelAction(Runnable action, CompletableFuture<?>
fut) {
}
}
- boolean isCancelled() {
+ @Override
+ public boolean isCancelled() {
return cancelFut != null;
}
+ @Override
+ public AutoCloseable listen(Runnable callback) {
+ Objects.requireNonNull(callback, "callback");
+
+ if (cancelFut != null) {
+ callback.run();
+ return () -> { };
+ }
Review Comment:
When the token is already cancelled, `listen(...)` invokes `callback.run()`
synchronously and lets any exception escape directly to the caller. When the
callback is invoked during `cancel()`, exceptions are caught and
wrapped/suppressed into an `IgniteException`. This makes listener error
behavior depend on timing. Consider defining and enforcing one consistent
error-handling contract (e.g., always catching/wrapping, or always propagating)
and aligning both paths to it; otherwise users can see different exception
types for the same callback depending on race timing.
##########
modules/api/src/main/java/org/apache/ignite/lang/CancelHandleImpl.java:
##########
@@ -69,6 +72,8 @@ static final class CancellationTokenImpl implements
CancellationToken {
private final ArrayDeque<Cancellation> cancellations = new
ArrayDeque<>();
+ private final List<Runnable> listeners = new ArrayList<>();
Review Comment:
Listener callbacks are copied and executed on cancel, but the backing
`listeners` list is never cleared. After cancellation, the token will retain
strong references to all registered callbacks indefinitely, which creates a
concrete memory-retention issue (including for callbacks that capture large
object graphs). Consider clearing `listeners` under `mux` after creating
`listenersCopy` (or otherwise dropping references) once cancellation is
requested.
##########
modules/api/src/main/java/org/apache/ignite/lang/CancelHandleImpl.java:
##########
@@ -96,12 +101,39 @@ void addCancelAction(Runnable action, CompletableFuture<?>
fut) {
}
}
- boolean isCancelled() {
+ @Override
+ public boolean isCancelled() {
return cancelFut != null;
}
+ @Override
+ public AutoCloseable listen(Runnable callback) {
+ Objects.requireNonNull(callback, "callback");
+
+ if (cancelFut != null) {
+ callback.run();
+ return () -> { };
+ }
+
+ synchronized (mux) {
+ if (cancelFut == null) {
+ listeners.add(callback);
+ return () -> {
+ synchronized (mux) {
+ listeners.remove(callback);
+ }
+ };
+ }
+ }
+
+ callback.run();
+ return () -> { };
+ }
+
@SuppressWarnings("rawtypes")
void cancel() {
+ List<Runnable> listenersCopy;
Review Comment:
Listener callbacks are copied and executed on cancel, but the backing
`listeners` list is never cleared. After cancellation, the token will retain
strong references to all registered callbacks indefinitely, which creates a
concrete memory-retention issue (including for callbacks that capture large
object graphs). Consider clearing `listeners` under `mux` after creating
`listenersCopy` (or otherwise dropping references) once cancellation is
requested.
##########
modules/api/src/main/java/org/apache/ignite/lang/CancellationToken.java:
##########
@@ -22,4 +22,24 @@
* to terminate it.
*/
public interface CancellationToken {
+ /**
+ * Flag indicating whether cancellation was requested or not.
+ *
+ * <p>This method will return {@code true} even if cancellation has not
been completed yet.
+ *
+ * @return {@code true} when cancellation was requested.
+ */
+ boolean isCancelled();
+
+ /**
+ * Registers a callback to be executed when cancellation is requested. If
cancellation has already been requested,
+ * the callback is executed immediately.
+ *
+ * <p>The returned handle can be used to stop listening for cancellation
requests. It is important to close the handle
+ * when the callback is no longer needed to avoid memory leaks.
+ *
+ * @param callback Callback to execute when cancellation is requested.
+ * @return A handle which can be used to stop listening for cancellation
requests.
+ */
+ AutoCloseable listen(Runnable callback);
Review Comment:
Adding new abstract methods to a public interface is a source/binary
breaking change for any external implementations of `CancellationToken`. If
Ignite’s compatibility policy requires non-breaking API evolution, consider
providing `default` implementations (e.g., `default boolean isCancelled() {
return false; }` and `default AutoCloseable listen(...) { ... }`) or
introducing a new extended interface to avoid breaking existing implementers.
```suggestion
default AutoCloseable listen(Runnable callback) {
if (isCancelled()) {
callback.run();
}
// Default implementation does not register for future notifications
and returns
// a no-op handle. Implementations that support listening should
override this method.
return () -> {
// no-op
};
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
