You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Ngone51 (via GitHub)" <gi...@apache.org> on 2025/08/27 04:50:47 UTC
[PR] [SPARK-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Ngone51 opened a new pull request, #52139:
URL: https://github.com/apache/spark/pull/52139
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR fixes `UninterruptibleLock.isInterruptible` to avoid duplicated interruption when the thread is already interrupted.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
The "uninterruptible" semantic of `UninterruptibleThread`is broken (i.e., `UninterruptibleThread` is interruptible even if it's under `runUninterruptibly`) after the fix https://github.com/apache/spark/pull/50594. The probelm is that the state of
`shouldInterruptThread` becomes unsafe when there are multiple interrupts concurrently.
For example, thread A could interrupt UninterruptibleThread ut first before UninterruptibleThread enters `runUninterruptibly`. Right after that, another thread B starts to invoke ut.interrupt() and pass through `uninterruptibleLock.isInterruptible` (becasue at this point, `shouldInterruptThread = uninterruptible = false`). Before thread B invokes `super.interrupt()`, UninterruptibleThread ut enters `runUninterruptibly` and pass through `uninterruptibleLock.getAndSetUninterruptible` and set `uninterruptible = true`. Then, thread ut continues the check `uninterruptibleLock.isInterruptPending`. However, `uninterruptibleLock.isInterruptPending` return false at this point (due to `shouldInterruptThread = Thread.interrupted = true`) even though thread B is actully interrupting. *As a result, the state of `shouldInterruptThread` becomes inconsistent between thread B and thread ut.* Then, as `uninterruptibleLock.isInterruptPending` returns false, ut to continute to execute `f`. At the sam
e time, thread B invokes `super.interrupt()`, and `f` could be interrupted
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Manually tested. The issue can be easily reproduced if we run `UninterruptibleThreadSuite.stress test` for 100 times in a row:
```
[info] true did not equal false (UninterruptibleThreadSuite.scala:208)
[info] org.scalatest.exceptions.TestFailedException:
[info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info] at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info] at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info] at org.apache.spark.util.UninterruptibleThreadSuite.$anonfun$new$7(UninterruptibleThreadSuite.scala:208)
...
```
And the issue is gone after the fix.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
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: 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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2306104694
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
Upated. Please take another 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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3230849105
> @Ngone51 What is the assumption? If thread A or thread B interrupts ut before it calls uninterruptibleLock.getAndSetUninterruptible(true), ut can be interrupted.
@vrozov Thread ut is interrupted in this case. But it continous the execution because there is no `InterruptedException` triggered.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "vrozov (via GitHub)" <gi...@apache.org>.
vrozov commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3228749448
@Ngone51 What is the assumption? If thread A or thread B interrupts `ut` before it calls `uninterruptibleLock.getAndSetUninterruptible(true)`, `ut` can be interrupted.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "vrozov (via GitHub)" <gi...@apache.org>.
vrozov commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2304469712
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
I don't think it is necessary to use `UninterruptibleThread.this` and you may use `isInterrupted`.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2303060067
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -100,8 +100,12 @@ private[spark] class UninterruptibleThread(
* @return true when there is no concurrent [[runUninterruptibly()]] call ([[uninterruptible]]
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
- def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ def isInterruptible(t: Thread): Boolean = synchronized {
Review Comment:
Good idea!
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3233723156
Yes, it still works.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3226940457
Can the existing tests verify this fix?
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2305839692
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
We can't. Because we are in the inner class `UninterruptibleLock` rather than `UninterruptibleThread`.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #52139: [SPARK-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt
URL: https://github.com/apache/spark/pull/52139
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "vrozov (via GitHub)" <gi...@apache.org>.
vrozov commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2305322051
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
Review Comment:
nit: this is code comment, not a doc comment, using Markdown formatting reduces readability IMO.
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
I think `isInterrupted` can be moved to line 115:
```
if (!shouldInterruptThread && !awaitInterruptThread && !isInterrupted) {
...
```
along with updating comment "there is no other threads that called interrupt" to cover both conditions: "awaitInterruptThread is already true or isInterrupted is true.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "vrozov (via GitHub)" <gi...@apache.org>.
vrozov commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2306021044
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
It is not so much about readability. I think it is not necessary to set `shouldInterruptThread` based on `isInterrupted` and it should be used just to skip calling `super.interrupt()` (similar how `awaitInterruptThread` is used).
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "vrozov (via GitHub)" <gi...@apache.org>.
vrozov commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2306009100
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
In Scala you can directly access outer class methods without using `Outer.this` (that would be necessary in Java). Can you give it a try (I checked and it compiles and runs)?
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3227013017
> Can the existing tests verify this fix?
@cloud-fan Yes. `UninterruptibleThreadSuite.stress test` could esaily fail if we run it for 100 times in a row. But it won't after the fix.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3226729944
cc @mridulm @cloud-fan @vrozov
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3235534448
thanks, merging to master/4.0!
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2305919594
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
TBH, I think that would be less readable.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2303004261
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -100,8 +100,12 @@ private[spark] class UninterruptibleThread(
* @return true when there is no concurrent [[runUninterruptibly()]] call ([[uninterruptible]]
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
- def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ def isInterruptible(t: Thread): Boolean = synchronized {
Review Comment:
I think in scala we can just do `UninterruptibleThread.this`, then we don't need to take a new parameter in this function.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2303002552
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -100,8 +100,12 @@ private[spark] class UninterruptibleThread(
* @return true when there is no concurrent [[runUninterruptibly()]] call ([[uninterruptible]]
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
- def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ def isInterruptible(t: Thread): Boolean = synchronized {
Review Comment:
This is in an inner class of `UninterruptibleThread`, can we access the outer `this` directly?
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3233000081
@Ngone51 since this needs manual test, have you verified the new fix locally?
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #52139:
URL: https://github.com/apache/spark/pull/52139#discussion_r2306104382
##########
core/src/main/scala/org/apache/spark/util/UninterruptibleThread.scala:
##########
@@ -101,7 +101,11 @@ private[spark] class UninterruptibleThread(
* is true) and no concurrent [[interrupt()]] call, otherwise false
*/
def isInterruptible: Boolean = synchronized {
- shouldInterruptThread = uninterruptible
+ // SPARK-53394: We should not interrupt the thread when it is already interrupted.
+ // Otherwise, the state of `shouldInterruptThread` becomes inconsistent between
+ // `isInterruptible()` and `isInterruptPending()`, leading to `UninterruptibleThread`
+ // be interruptible under `runUninterruptibly`.
+ shouldInterruptThread = uninterruptible || UninterruptibleThread.this.isInterrupted
Review Comment:
Yea, I think you are right. Updated the code.
--
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-53394][CORE] UninterruptibleLock.isInterruptible should avoid duplicated interrupt [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on PR #52139:
URL: https://github.com/apache/spark/pull/52139#issuecomment-3235552278
Thanks! @cloud-fan @vrozov
--
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