emecii commented on code in PR #987:
URL: https://github.com/apache/flink-agents/pull/987#discussion_r3793777015
##########
runtime/src/main/java/org/apache/flink/agents/runtime/ResourceCache.java:
##########
@@ -140,32 +141,32 @@ public void put(String name, ResourceType type, Resource
resource) {
@Override
public void close() throws Exception {
- Exception firstException = null;
+ // Close every cached resource, then the resource context, even when
an earlier close
+ // fails. The first failure is rethrown with the later ones suppressed.
+ //
+ // The ladders catch Throwable, not Exception:
ActionExecutionOperator.close() closes this
+ // cache before the Python interpreter because cached resources may
hold Python references,
+ // so a non-Exception Throwable escaping here would leave the
remaining resources open
+ // while the interpreter behind them is torn down anyway.
ExceptionUtils.rethrowException
+ // passes Error and Exception through unchanged, so the caller still
sees the original.
+ Throwable firstFailure = null;
for (Map<String, Resource> resources : cache.values()) {
Review Comment:
Agreed — aligned in 23312147, with the same `first_closed=True,
second_closed=False` behavior as your reproduction. Python now attempts every
resource, always reaches `_cache.clear()` and `_resource_context.close()`, and
re-raises the first failure.
Two deliberate differences from the Java side, documented in the docstring
rather than left for a reader to notice:
- **`Exception`, not the literal analogue of `Throwable`.** The Java
argument does not carry over: Python's `Exception` already covers what Java
calls `Error` (`MemoryError`, `RecursionError`), while `BaseException` would
additionally swallow `KeyboardInterrupt` and `SystemExit`, which cleanup must
not do. So the parity is in the contract, not the catch clause.
- **Later failures are logged, not attached.** `ExceptionGroup` needs 3.11
and `requires-python` is `>=3.10,<3.13`, so there is no real equivalent of
`addSuppressed` available.
Four tests in
`python/flink_agents/runtime/tests/test_resource_cache_close.py` covering the
failing-resource path, first-failure-wins, a resource-context failure, and the
healthy path. Restoring the sequential version fails the first two.
Two related things I looked at and deliberately did not change, so they are
visible rather than silently skipped:
- `SkillManager.close()` on the Python side uses
`contextlib.suppress(Exception)` per repo. It already closes every repo, so the
close-all property holds, but it reports nothing at all — the mirror image of
the Java gap. Changing swallow → raise is a behavior change for its callers, so
it seemed wrong to fold into this PR uninvited. Happy to do it here or file it,
whichever you prefer.
- `FlinkRunnerContext.close()` above it already sequences its two components
correctly.
--
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]
