emecii commented on code in PR #987:
URL: https://github.com/apache/flink-agents/pull/987#discussion_r3744750625
##########
runtime/src/test/java/org/apache/flink/agents/runtime/operator/PythonBridgeManagerTest.java:
##########
@@ -59,4 +67,92 @@ void openIsNoOpWhenPlanHasNeitherPythonActionsNorResources()
throws Exception {
assertThat(bridge.getPythonRunnerContext()).isNull();
}
}
+
+ /**
+ * A failing action executor must not strand the interpreter or the
environment manager: both
+ * hold native Python state that leaks for the lifetime of the TaskManager
if never closed.
+ */
+ @Test
+ void closeReleasesInterpreterAndEnvironmentWhenActionExecutorFails()
throws Exception {
+ PythonBridgeManager bridge = new PythonBridgeManager();
+ PythonActionExecutor actionExecutor = mock(PythonActionExecutor.class);
+ PythonInterpreter interpreter = mock(PythonInterpreter.class);
+ PythonEnvironmentManager environmentManager =
mock(PythonEnvironmentManager.class);
+ doThrow(new IllegalStateException("action executor close failed"))
+ .when(actionExecutor)
+ .close();
+
+ setField(bridge, "pythonActionExecutor", actionExecutor);
+ setField(bridge, "pythonInterpreter", interpreter);
+ setField(bridge, "pythonEnvironmentManager", environmentManager);
+
+ assertThatThrownBy(bridge::close)
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessage("action executor close failed");
+
+ verify(interpreter).close();
+ verify(environmentManager).close();
Review Comment:
Worth it — added in 9b5688d0, using your snippet in
`closeReleasesInterpreterAndEnvironmentWhenActionExecutorFails`, with a javadoc
note on why the order is load-bearing rather than incidental (the class javadoc
at `:70-71`, and `PythonActionExecutor.close()` calling back into the
interpreter at `:205-219`). Your swap to `{pythonInterpreter,
pythonActionExecutor, pythonEnvironmentManager}` now fails that test; it passed
before.
The clause in contract 1 was meant to carry that weight, so this closes the
gap between what it claims and what was checked.
Reviewing the rest of the diff against the same question,
`ActionExecutionOperator.close()` had the identical hole and a more explicitly
documented constraint — `resourceCache` must close before `pythonBridge`
because cached resources may hold Python references (the comment at `:556`).
Swapping those two left both operator close tests green. They now use one
`InOrder` chain across all five components plus `stateHandler.dispose()`, so
the same swap fails there too, and `super.close()` is pinned last in the same
assertion.
--
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]
