Github user nivox commented on a diff in the pull request:
https://github.com/apache/spark/pull/19217#discussion_r146518809
--- Diff:
launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
@@ -232,20 +232,20 @@ public void run() {
};
ServerConnection clientConnection = new ServerConnection(client,
timeout);
Thread clientThread = factory.newThread(clientConnection);
- synchronized (timeout) {
- clientThread.start();
- synchronized (clients) {
- clients.add(clientConnection);
- }
- long timeoutMs = getConnectionTimeout();
- // 0 is used for testing to avoid issues with clock resolution /
thread scheduling,
- // and force an immediate timeout.
- if (timeoutMs > 0) {
- timeoutTimer.schedule(timeout, getConnectionTimeout());
- } else {
- timeout.run();
- }
+ synchronized (clients) {
+ clients.add(clientConnection);
--- End diff --
Adding the connection to `clients` *before* starting the thread should be
safer since otherwise the thread could have a chance of running and terminating
*before* the connection is added to `clients`. This would cause the addition of
an already terminated connection to the `clients` list which nobody would ever
cleanup (i.e. memory leak).
This situation is really unlikely but still a possibility.
Changing the order of operation shouldn't affect any logic since the
`clients` list is only used for cleanup in the `close` method.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
