HwangRock opened a new pull request, #5310:
URL: https://github.com/apache/zeppelin/pull/5310
### What is this PR for?
`DockerInterpreterProcess.stop()` calls `docker.close()` after the
`killContainer` / `removeContainer` try-catch, but `close()` is outside that
block and the `catch` only handles `InterruptedException` and `DockerException`:
```java
try {
docker.killContainer(containerName);
docker.removeContainer(containerName);
} catch (InterruptedException e) { ...; Thread.currentThread().interrupt(); }
catch (DockerException e) { ... }
docker.close(); // skipped if kill/remove throws anything else
```
If `killContainer` / `removeContainer` throws an exception that is neither
of those two (e.g. an unchecked `RuntimeException` from the client), the
exception propagates out of `stop()` and `docker.close()` is skipped. The
`DockerClient` holds an HTTP connection to the Docker daemon, so its socket and
file descriptors leak. Interpreters are started and stopped repeatedly, so the
leak accumulates and can exhaust file descriptors ("Too many open files") on
long-running servers.
Fix: move `docker.close()` into a `finally` block so the client is always
closed regardless of how `killContainer` / `removeContainer` exit.
### What type of PR is it?
Bug Fix
### Todos
* [ ] - none
### What is the Jira issue?
* https://issues.apache.org/jira/browse/ZEPPELIN-6537
### How should this be tested?
Added `testStopClosesDockerClientEvenWhenKillContainerThrows` in
`DockerInterpreterProcessTest`: it injects a mock `DockerClient`, stubs
`killContainer` to throw a `RuntimeException`, calls `stop()`, and verifies the
exception still propagates (`assertThrows`) while `docker.close()` is still
invoked (`verify(mockDocker).close()`).
On the old code this fails with "Wanted but not invoked:
dockerClient.close()"; with the fix it passes. Deterministic (mock-based), no
flakiness. Full `DockerInterpreterProcessTest` passes.
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? No.
* Is there breaking changes for older versions? No — behavior only changes
on the failure path (close now always runs); the exception still propagates as
before.
* Does this needs documentation? No.
--
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]
