emecii commented on code in PR #987:
URL: https://github.com/apache/flink-agents/pull/987#discussion_r3744749319
##########
runtime/src/main/java/org/apache/flink/agents/runtime/operator/ActionExecutionOperator.java:
##########
@@ -553,24 +553,38 @@ public void waitInFlightEventsFinished() throws Exception
{
@Override
public void close() throws Exception {
- // Must close before pythonInterpreter since cached resources may hold
Python references.
- if (resourceCache != null) {
- resourceCache.close();
- }
- if (contextManager != null) {
- contextManager.close();
- }
- if (pythonBridge != null) {
- pythonBridge.close();
- }
- if (eventRouter != null) {
- eventRouter.close();
+ // Close every component even when an earlier one fails, so a failing
close cannot leak
+ // the components behind it or skip super.close(). The first failure
is rethrown with
+ // the later ones suppressed. Order is preserved: the resource cache
must close before
+ // pythonInterpreter since cached resources may hold Python references.
+ //
+ // The ladder catches Throwable, not Exception, and IOUtils.closeAll
is deliberately not
+ // used: both stop at the first non-Exception Throwable without
closing what follows,
+ // which is the very leak this method has to avoid.
+ Throwable firstFailure = null;
+ for (AutoCloseable closeable :
+ new AutoCloseable[] {
+ resourceCache, contextManager, pythonBridge, eventRouter,
durableExecManager
Review Comment:
Good catch — taken in scope, fixed in 9b5688d0.
Not intentional on my part. And I think it is slightly worse than "not a
regression": this patch makes that path more consequential rather than less.
Before, an `Error` out of a cached `Resource.close()` skipped
`pythonBridge.close()` entirely, so nothing was torn down out of order. Now the
ladder continues and closes the interpreter while those cached resources are
still open — which inverts the very ordering the comment at `:556` exists to
preserve. Widening `ResourceCache` restores it, so the two changes belong in
the same patch.
`ResourceCache.close()` now uses the same shape as the other three:
`Throwable` ladders, `ExceptionUtils.firstOrSuppressed`,
`ExceptionUtils.rethrowException`. Its only production caller is the operator
ladder, which already catches `Throwable`, so nothing downstream changes.
Two tests in `ResourceCacheTest`. The `Error` one pins all three
consequences you listed, including the two I would otherwise have been assuming
from position in the method rather than observing: `cache.clear()` is checked
by reflecting the cache map, and `resourceContext.close()` by standing a mock
`SkillManager` into the context, since `ResourceContextImpl.close()` closes it.
Narrowing the catch back to `Exception` fails that test.
I also updated the PR body, which cited `ResourceCache.close()` as untouched
prior art for the aggregation shape — no longer accurate now that this patch
modifies it.
--
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]
