[SPARK-6209] Clean up connections in ExecutorClassLoader after failing to load classes (master branch PR) #4944

JoshRosen · 2015-03-09T04:26:59Z

ExecutorClassLoader does not ensure proper cleanup of network connections that it opens. If it fails to load a class, it may leak partially-consumed InputStreams that are connected to the REPL's HTTP class server, causing that server to exhaust its thread pool, which can cause the entire job to hang. See SPARK-6209 for more details, including a bug reproduction.

This patch fixes this issue by ensuring proper cleanup of these resources. It also adds logging for unexpected error cases.

This PR is an extended version of #4935 and adds a regression test.

SparkQA · 2015-03-09T04:27:45Z

Test build #28378 has started for PR 4944 at commit 961c284.

This patch merges cleanly.

SparkQA · 2015-03-09T05:52:40Z

Test build #28378 has finished for PR 4944 at commit 961c284.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-09T05:52:44Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28378/
Test PASSed.

JoshRosen · 2015-03-11T18:08:22Z

/cc @mccheah @pwendell @holdenk @andrewor14

mccheah · 2015-03-11T18:42:40Z

This looks like the right thing to do. Thanks for submitting this!

@ash211 @mingyukim care to take a look?

mccheah · 2015-03-11T21:19:53Z

repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala

+      connection.setConnectTimeout(httpUrlConnectionTimeoutMillis)
+      connection.setReadTimeout(httpUrlConnectionTimeoutMillis)
+    }
+    connection.connect()


I'm not too familiar with this API - but can this connection object ever leak even if the returned input-stream is closed? Or, can an exception occur before returning from this method and the connection leak that way? I'm guessing these aren't possible but just wanted to educate myself as to how these scenarios won't happen =)

Per the HttpURLConnection Javadoc:

Each HttpURLConnection instance is used to make a single request but the underlying network connection to the HTTP server may be transparently shared by other instances. Calling the close() methods on the InputStream or OutputStream of an HttpURLConnection after a request may free network resources associated with this instance but has no effect on any shared persistent connection. Calling the disconnect() method may close the underlying socket if a persistent connection is otherwise idle at that time.

This can cause problems in practice: https://scotte.github.io/2015/01/httpurlconnection-socket-leak/.

Actually, that blog post reminds me that I should probably call getErrorStream and read that stream in the next error-handling block prior to calling disconnect.

On closer inspection, it looks like you need to consume the errorStream if you want the underlying connection to be eligible for re-use, but we may not necessarily care about that here: http://docs.oracle.com/javase/6/docs/technotes/guides/net/http-keepalive.html

How about if an exception throws before we return the input stream? In that case the input stream is not closed but perhaps the connection is still open?

I'm basically asking, should we have a catch block for Throwable in this method after we instantiate the connection to close the connection if we run into an Exception before returning the input stream?

I think that the connect() call is unlikely to fail in ways that leak resources, but suppose it wouldn't hurt to add another try-catch block just to be safe. I can do this tomorrow.

… error stream

SparkQA · 2015-03-15T21:28:09Z

Test build #28633 has started for PR 4944 at commit e0e3c25.

This patch merges cleanly.

JoshRosen · 2015-03-15T22:27:09Z

I've updated this with a new commit that adds another layer of try-catch blocks to handle errors when reading the status code. I also made a slight change to how we handle 404 errors: we now attempt to close the error stream and only call disconnect() as a last-resort in case the error stream cleanup fails. This allows us to continue to benefit from HTTP keep-alive / persistent connections even when errors occur (I verified this behavior using Wireshark).

SparkQA · 2015-03-15T22:49:47Z

Test build #28633 has finished for PR 4944 at commit e0e3c25.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-15T22:49:51Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28633/
Test PASSed.

JoshRosen · 2015-03-17T16:31:00Z

I think this should be good to merge, but I'm going to leave it open for a little while longer in order to allow others to comment.

JoshRosen · 2015-03-24T00:29:24Z

Ping @andrewor14 (as requested).

mccheah · 2015-03-24T00:45:38Z

Yeah it looks okay to me but I also would feel more comfortable if a second core committer took a look.

andrewor14 · 2015-03-24T21:36:39Z

LGTM

andrewor14 · 2015-03-24T21:37:27Z

This goes into master and 1.3.

…g to load classes (master branch PR) ExecutorClassLoader does not ensure proper cleanup of network connections that it opens. If it fails to load a class, it may leak partially-consumed InputStreams that are connected to the REPL's HTTP class server, causing that server to exhaust its thread pool, which can cause the entire job to hang. See [SPARK-6209](https://issues.apache.org/jira/browse/SPARK-6209) for more details, including a bug reproduction. This patch fixes this issue by ensuring proper cleanup of these resources. It also adds logging for unexpected error cases. This PR is an extended version of #4935 and adds a regression test. Author: Josh Rosen <joshrosen@databricks.com> Closes #4944 from JoshRosen/executorclassloader-leak-master-branch and squashes the following commits: e0e3c25 [Josh Rosen] Wrap try block around getReponseCode; re-enable keep-alive by closing error stream 961c284 [Josh Rosen] Roll back changes that were added to get the regression test to fail 7ee2261 [Josh Rosen] Add a failing regression test e2d70a3 [Josh Rosen] Properly clean up after errors in ExecutorClassLoader (cherry picked from commit 7215aa7) Signed-off-by: Andrew Or <andrew@databricks.com>

JoshRosen · 2015-03-24T21:49:48Z

Thanks for the review. I'll open a separate PR for branch-1.2 (1.2.2).

…g to load classes (master branch PR) ExecutorClassLoader does not ensure proper cleanup of network connections that it opens. If it fails to load a class, it may leak partially-consumed InputStreams that are connected to the REPL's HTTP class server, causing that server to exhaust its thread pool, which can cause the entire job to hang. See [SPARK-6209](https://issues.apache.org/jira/browse/SPARK-6209) for more details, including a bug reproduction. This patch fixes this issue by ensuring proper cleanup of these resources. It also adds logging for unexpected error cases. This PR is an extended version of apache#4935 and adds a regression test. Author: Josh Rosen <joshrosen@databricks.com> Closes apache#4944 from JoshRosen/executorclassloader-leak-master-branch and squashes the following commits: e0e3c25 [Josh Rosen] Wrap try block around getReponseCode; re-enable keep-alive by closing error stream 961c284 [Josh Rosen] Roll back changes that were added to get the regression test to fail 7ee2261 [Josh Rosen] Add a failing regression test e2d70a3 [Josh Rosen] Properly clean up after errors in ExecutorClassLoader (cherry picked from commit 7215aa7) Signed-off-by: Andrew Or <andrew@databricks.com> Conflicts: repl/pom.xml repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala

JoshRosen · 2015-03-24T22:22:45Z

I've opened #5174 for branch-1.2 (1.2.2).

…g to load classes (branch-1.2) ExecutorClassLoader does not ensure proper cleanup of network connections that it opens. If it fails to load a class, it may leak partially-consumed InputStreams that are connected to the REPL's HTTP class server, causing that server to exhaust its thread pool, which can cause the entire job to hang. See [SPARK-6209](https://issues.apache.org/jira/browse/SPARK-6209) for more details, including a bug reproduction. This patch fixes this issue by ensuring proper cleanup of these resources. It also adds logging for unexpected error cases. (See #4944 for the corresponding PR for 1.3/1.4). Author: Josh Rosen <joshrosen@databricks.com> Closes #5174 from JoshRosen/executorclassloaderleak-branch-1.2 and squashes the following commits: 16e38fe [Josh Rosen] [SPARK-6209] Clean up connections in ExecutorClassLoader after failing to load classes (master branch PR)

…g to load classes (branch-1.2) ExecutorClassLoader does not ensure proper cleanup of network connections that it opens. If it fails to load a class, it may leak partially-consumed InputStreams that are connected to the REPL's HTTP class server, causing that server to exhaust its thread pool, which can cause the entire job to hang. See [SPARK-6209](https://issues.apache.org/jira/browse/SPARK-6209) for more details, including a bug reproduction. This patch fixes this issue by ensuring proper cleanup of these resources. It also adds logging for unexpected error cases. (See apache#4944 for the corresponding PR for 1.3/1.4). Author: Josh Rosen <joshrosen@databricks.com> Closes apache#5174 from JoshRosen/executorclassloaderleak-branch-1.2 and squashes the following commits: 16e38fe [Josh Rosen] [SPARK-6209] Clean up connections in ExecutorClassLoader after failing to load classes (master branch PR)

…imeoutMillis` from `ExecutorClassLoader` ### What changes were proposed in this pull request? This pr aims to remove unused `private[executor] var httpUrlConnectionTimeoutMillis` from `ExecutorClassLoader`. The definition of `httpUrlConnectionTimeoutMillis` was introduced in SPARK-6209 (#4944) and was used by the function `getClassFileInputStreamFromHttpServer`. After SPARK-23538(#20723), the function `getClassFileInputStreamFromHttpServer` was removed from `ExecutorClassLoader`, and `httpUrlConnectionTimeoutMillis` is no longer used either. ### Why are the changes needed? Code cleanup ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GitHub Action ### Was this patch authored or co-authored using generative AI tooling? No Closes #50152 from LuciferYang/minor-ExecutorClassLoader-var. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…imeoutMillis` from `ExecutorClassLoader` ### What changes were proposed in this pull request? This pr aims to remove unused `private[executor] var httpUrlConnectionTimeoutMillis` from `ExecutorClassLoader`. The definition of `httpUrlConnectionTimeoutMillis` was introduced in SPARK-6209 (apache#4944) and was used by the function `getClassFileInputStreamFromHttpServer`. After SPARK-23538(apache#20723), the function `getClassFileInputStreamFromHttpServer` was removed from `ExecutorClassLoader`, and `httpUrlConnectionTimeoutMillis` is no longer used either. ### Why are the changes needed? Code cleanup ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GitHub Action ### Was this patch authored or co-authored using generative AI tooling? No Closes apache#50152 from LuciferYang/minor-ExecutorClassLoader-var. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

JoshRosen added 3 commits March 8, 2015 19:43

Properly clean up after errors in ExecutorClassLoader

e2d70a3

Add a failing regression test

7ee2261

Roll back changes that were added to get the regression test to fail

961c284

JoshRosen mentioned this pull request Mar 9, 2015

[SPARK-6209] Clean up connections in ExecutorClassLoader after failing to load classes #4935

Closed

mccheah reviewed Mar 11, 2015
View reviewed changes

Wrap try block around getReponseCode; re-enable keep-alive by closing…

e0e3c25

… error stream

asfgit closed this in 7215aa7 Mar 24, 2015

JoshRosen mentioned this pull request Mar 24, 2015

[SPARK-6209] Clean up connections in ExecutorClassLoader after failing to load classes (branch-1.2) #5174

Closed

LuciferYang mentioned this pull request Mar 4, 2025

[MINOR][CORE] Remove unused private[executor] var httpUrlConnectionTimeoutMillis from ExecutorClassLoader #50152

Closed

[SPARK-6209] Clean up connections in ExecutorClassLoader after failing to load classes (master branch PR) #4944

[SPARK-6209] Clean up connections in ExecutorClassLoader after failing to load classes (master branch PR) #4944

Uh oh!

Conversation

JoshRosen commented Mar 9, 2015

Uh oh!

SparkQA commented Mar 9, 2015

Uh oh!

SparkQA commented Mar 9, 2015

Uh oh!

AmplabJenkins commented Mar 9, 2015

Uh oh!

JoshRosen commented Mar 11, 2015

Uh oh!

mccheah commented Mar 11, 2015

Uh oh!

mccheah Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

JoshRosen Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

JoshRosen Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

mccheah Mar 11, 2015

Choose a reason for hiding this comment

Uh oh!

JoshRosen Mar 13, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 15, 2015

Uh oh!

JoshRosen commented Mar 15, 2015

Uh oh!

SparkQA commented Mar 15, 2015

Uh oh!

AmplabJenkins commented Mar 15, 2015

Uh oh!

JoshRosen commented Mar 17, 2015

Uh oh!

JoshRosen commented Mar 24, 2015

Uh oh!

mccheah commented Mar 24, 2015

Uh oh!

andrewor14 commented Mar 24, 2015

Uh oh!

andrewor14 commented Mar 24, 2015

Uh oh!

JoshRosen commented Mar 24, 2015

Uh oh!

JoshRosen commented Mar 24, 2015

Uh oh!

Uh oh!