You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "cloud-fan (via GitHub)" <gi...@apache.org> on 2026/05/27 11:06:38 UTC
[PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Avoid blocking process exit when server is unreachable in SparkConnectClient._on_exit [spark]
cloud-fan opened a new pull request, #56140:
URL: https://github.com/apache/spark/pull/56140
### What changes were proposed in this pull request?
Follow-up to #54106. Two changes to `SparkConnectClient._on_exit`:
1. Bail out immediately if `self._closed` is already `True`, so we do not make any gRPC calls (including the unconditional `_cleanup_ml_cache()`) after the client has been explicitly closed.
2. Run the remaining cleanup work in a daemon thread joined with a 5-second timeout, so process exit is not blocked when the server is unreachable. A daemon thread is abandoned during interpreter shutdown if it does not complete within the timeout.
### Why are the changes needed?
`_on_exit` is registered with `atexit` and runs during process shutdown. It unconditionally calls `_cleanup_ml_cache()`, which issues a gRPC `ExecutePlan` call. If the server is unreachable, that call can block for a long time on the gRPC connection timeout before raising — making the Python process appear to hang on exit, both in tests and in production scenarios where the server goes down before the client.
The same hang risk applies to `release_session()` and `close()` in the opt-in `SPARK_CONNECT_RELEASE_SESSION_ON_EXIT` branch.
See https://github.com/apache/spark/pull/54106#issuecomment-4079259041 for context.
### Does this PR introduce _any_ user-facing change?
No. Behavior change is limited to making process exit non-blocking when the Spark Connect server is unreachable.
### How was this patch tested?
Existing PySpark Connect tests.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.7)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Avoid blocking process exit when server is unreachable in SparkConnectClient._on_exit [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4553971096
cc @wbo4958 @HyukjinKwon
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Avoid blocking process exit when server is unreachable in SparkConnectClient._on_exit [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4558257426
Hi @cloud-fan 's agent! This is a larger change now because `execute_command` is basically skipped and raw command is directly sent. We need the confirmation from @cloud-fan whether this is what we want.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Avoid blocking process exit when server is unreachable in SparkConnectClient._on_exit [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4558854411
Yeah, my agent also suggested a similar thing but I ended up with not opening a PR cuz I wasn't sure.
@wbo4958 can you take a quick look?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Skip cleanup RPCs in _on_exit when client is already closed [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4563069285
Thanks for catching that, @gaogaotiantian — you're right that bypassing `execute_command` is a bigger change than I'd like to land without more discussion.
I've rescoped this PR to only the uncontroversial fix: short-circuit `_on_exit` when `self._closed` is already `True`. That covers the case where the user has explicitly called `close()` but the `atexit` handler still re-issues a cleanup RPC afterward — which is wasted work and a hang risk if the server is unreachable.
The broader case (user never called `close()`, server is unreachable) is left for a separate PR where we can decide whether bypassing the reattachable iterator for fire-and-forget cleanup is acceptable, or whether to thread a deadline through `execute_command` instead. cc @wbo4958 @HyukjinKwon
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Avoid blocking process exit when server is unreachable in SparkConnectClient._on_exit [spark]
Posted by "gaogaotiantian (via GitHub)" <gi...@apache.org>.
gaogaotiantian commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4557767067
You can't start a thread in `atexit` registered functions - https://docs.python.org/3/library/atexit.html#atexit.register.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Skip cleanup RPCs in _on_exit when client is already closed [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4589967287
thanks for the review, merging to master/4.x/4.2 (the commit to followup is in 4.2)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Skip cleanup RPCs in _on_exit when client is already closed [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #56140: [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Skip cleanup RPCs in _on_exit when client is already closed
URL: https://github.com/apache/spark/pull/56140
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Avoid blocking process exit when server is unreachable in SparkConnectClient._on_exit [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4557983896
Thanks @gaogaotiantian, you're absolutely right — I missed that. The [atexit docs](https://docs.python.org/3/library/atexit.html#atexit.register) explicitly forbid starting threads in registered functions (raises `RuntimeError` on 3.12+, risks race conditions on earlier versions), so the daemon-thread approach was broken on the very platform Spark targets.
I've pushed a revision that drops the thread entirely and uses gRPC's native per-call deadline instead — `release_session` and `_cleanup_ml_cache` now accept an optional `timeout` that's passed through to the stub call, and `_on_exit` invokes them with `timeout=10.0`. If the deadline elapses, the RPC fails with `DEADLINE_EXCEEDED`, we swallow it, and the server's own session GC reclaims any leftover state — same end behavior, no thread involved. PR description updated accordingly.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55326][PYTHON][CONNECT][FOLLOWUP] Skip cleanup RPCs in _on_exit when client is already closed [spark]
Posted by "wbo4958 (via GitHub)" <gi...@apache.org>.
wbo4958 commented on PR #56140:
URL: https://github.com/apache/spark/pull/56140#issuecomment-4580627384
The PR looks pretty good to me. And I agree with a separate PR for the broader case from https://github.com/apache/spark/pull/56140#issuecomment-4563069285
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org