Clean up the leftover connection for finished threads in pinned thread mode by HyukjinKwon · Pull Request #471 · py4j/py4j

HyukjinKwon · 2022-03-07T12:08:57Z

What is the problem?

Correctly, there is resource leak when using the pinned thread mode (see also apache/spark#24898).

For example, if you repeat the codes below multiple times to create Py4J connections in multiple threads,

# PySpark application
import threading

def print_prop():
    # Py4J connection is used under the hood.
    print(spark.sparkContext.getLocalProperty("a"))

threading.Thread(target=print_prop).start()

the number of leftover connections grows:

spark._jvm._gateway_client.deque
deque([<py4j.clientserver.ClientServerConnection object at 0x7fdc60170940>, <py4j.clientserver.ClientServerConnection object at 0x7fdca011e760>, <py4j.clientserver.ClientServerConnection object at 0x7fdcb01acdc0>, <py4j.clientserver.ClientServerConnection object at 0x7fdc60170100>, <py4j.clientserver.ClientServerConnection object at 0x7fdcb0232d30>])

In the environment where multiple threads are used without a pool, it easily causes "Too many files open" due to the lack of file descriptors (as they are all occupied by unclosed sockets).

How do you fix?

This PR adds another variable to thread local that cleans up the connection right before the thread is finished. We need it as a separate thread local because connection is NOT cleaned because the reference is being held at JavaClient.deque.

See also 50fe45e for more details.

HyukjinKwon · 2022-03-07T12:09:27Z

cc @bartdag I badly wanted to make this fix. I am happy that I am finally able to submit this PR.

WeichenXu123 · 2022-03-08T01:36:29Z

@HyukjinKwon awesome work to fix this!

HyukjinKwon · 2022-03-10T06:47:54Z

it should be ready for a look now.

bartdag

Awesome work. It took me a while to understand what was causing the issue in the first place.

I made a few suggestions, but I do not feel strongly towards them. Feel free to take what you want from them!

Thinking about the root cause, in the future, we may want to add a method to allow threads to explicitly tell when their work is done. With the default gateway approach, you can always call GatewayClient.close() to close all connections, but this does not make sense in the pinned thread model (you want to close your connection, not all connections).

That being said, your solution is the only sane way to stay backward compatible, so kudos!!

py4j-python/src/py4j/clientserver.py

…d mode

HyukjinKwon · 2022-03-14T01:24:54Z

Merged!

### What changes were proposed in this pull request? This PR upgrade Py4J 0.10.9.4, with relevant documentation changes. ### Why are the changes needed? Py4J 0.10.9.3 has a resource leak issue when pinned thread mode is enabled - it's enabled by default in PySpark at 41af409. We worked around this by enforcing users to use `InheritableThread` or `inhteritable_thread_target` as a workaround. After upgrading, we don't need to enforce users anymore because it automatically cleans up, see also py4j/py4j#471 ### Does this PR introduce _any_ user-facing change? Yes, users don't have to use `InheritableThread` or `inhteritable_thread_target` to avoid resource leaking problem anymore. ### How was this patch tested? CI in this PR should test it out. Closes #35871 from HyukjinKwon/SPARK-38563. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 8193b40) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

### What changes were proposed in this pull request? This PR upgrade Py4J 0.10.9.4, with relevant documentation changes. ### Why are the changes needed? Py4J 0.10.9.3 has a resource leak issue when pinned thread mode is enabled - it's enabled by default in PySpark at 41af409. We worked around this by enforcing users to use `InheritableThread` or `inhteritable_thread_target` as a workaround. After upgrading, we don't need to enforce users anymore because it automatically cleans up, see also py4j/py4j#471 ### Does this PR introduce _any_ user-facing change? Yes, users don't have to use `InheritableThread` or `inhteritable_thread_target` to avoid resource leaking problem anymore. ### How was this patch tested? CI in this PR should test it out. Closes #35871 from HyukjinKwon/SPARK-38563. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

This PR upgrade Py4J 0.10.9.4, with relevant documentation changes. Py4J 0.10.9.3 has a resource leak issue when pinned thread mode is enabled - it's enabled by default in PySpark at 41af409. We worked around this by enforcing users to use `InheritableThread` or `inhteritable_thread_target` as a workaround. After upgrading, we don't need to enforce users anymore because it automatically cleans up, see also py4j/py4j#471 Yes, users don't have to use `InheritableThread` or `inhteritable_thread_target` to avoid resource leaking problem anymore. CI in this PR should test it out. Closes #35871 from HyukjinKwon/SPARK-38563. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 8193b40) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

HyukjinKwon force-pushed the cleanup-connection branch 9 times, most recently from 2060594 to c093d12 Compare March 8, 2022 06:16

HyukjinKwon marked this pull request as draft March 8, 2022 06:49

HyukjinKwon force-pushed the cleanup-connection branch from c093d12 to a05214e Compare March 10, 2022 05:10

HyukjinKwon marked this pull request as ready for review March 10, 2022 05:25

HyukjinKwon force-pushed the cleanup-connection branch from a05214e to 4b87d93 Compare March 10, 2022 06:47

bartdag reviewed Mar 10, 2022

View reviewed changes

py4j-python/src/py4j/clientserver.py Outdated Show resolved Hide resolved

py4j-python/src/py4j/clientserver.py Outdated Show resolved Hide resolved

py4j-python/src/py4j/clientserver.py Outdated Show resolved Hide resolved

HyukjinKwon force-pushed the cleanup-connection branch from 4b87d93 to 9e6525b Compare March 11, 2022 02:27

Clean up the leftover connection for finished threads in pinned threa…

ed5b97d

…d mode

HyukjinKwon force-pushed the cleanup-connection branch from 9e6525b to ed5b97d Compare March 11, 2022 10:49

HyukjinKwon merged commit 799ac4e into py4j:master Mar 14, 2022

HyukjinKwon added this to the 0.10.9.4 milestone Mar 16, 2022

HyukjinKwon mentioned this pull request Mar 16, 2022

[SPARK-38563][PYTHON] Upgrade to Py4J 0.10.9.4 apache/spark#35871

Closed

HyukjinKwon mentioned this pull request Jul 25, 2023

[SPARK-44509][CONNECT][FOLLOW-UP] Add inheritable support in new job cancellation API apache/spark#42149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up the leftover connection for finished threads in pinned thread mode#471

Clean up the leftover connection for finished threads in pinned thread mode#471
HyukjinKwon merged 1 commit intopy4j:masterfrom
HyukjinKwon:cleanup-connection

HyukjinKwon commented Mar 7, 2022 •

edited

Loading

Uh oh!

HyukjinKwon commented Mar 7, 2022

Uh oh!

WeichenXu123 commented Mar 8, 2022

Uh oh!

HyukjinKwon commented Mar 10, 2022

Uh oh!

bartdag left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HyukjinKwon commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

HyukjinKwon commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the problem?

How do you fix?

Uh oh!

HyukjinKwon commented Mar 7, 2022

Uh oh!

WeichenXu123 commented Mar 8, 2022

Uh oh!

HyukjinKwon commented Mar 10, 2022

Uh oh!

bartdag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HyukjinKwon commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HyukjinKwon commented Mar 7, 2022 •

edited

Loading