[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user rxin commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-61010433 Thanks. Merged in master branch-1.1. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/2957 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2957#discussion_r19455126
--- Diff: core/src/main/scala/org/apache/spark/FutureAction.scala ---
@@ -210,7 +210,9 @@ class ComplexFutureAction[T] extends FutureAction[T] {
} catch {
case e: Exception = p.failure(e)
} finally {
-thread = null
+ComplexFutureAction.this.synchronized {
--- End diff --
what effect does this synchronized do? it seems to me it is actually not
protecting anything
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/2957#discussion_r19455903
--- Diff: core/src/main/scala/org/apache/spark/FutureAction.scala ---
@@ -210,7 +210,9 @@ class ComplexFutureAction[T] extends FutureAction[T] {
} catch {
case e: Exception = p.failure(e)
} finally {
-thread = null
+ComplexFutureAction.this.synchronized {
--- End diff --
Here is the codes of `cancel`:
```Scala
1 override def cancel(): Unit = this.synchronized {
2_cancelled = true
3if (thread != null) {
4 thread.interrupt()
5}
6 }
```
Without `ComplexFutureAction.this.synchronized`, the codes can run in the
following order:
1. In thread T1, `cancel` is called.
2. After T1 finishes L3, it's suspended.
3. Assume T2 is the thread that runs the action. Now T2 is set `thread` to
null.
4. T1 is resumed and runs L4 (thread.interrupt()), because `thread` is
null, it will throw a NPE.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2957#discussion_r19503380
--- Diff: core/src/main/scala/org/apache/spark/FutureAction.scala ---
@@ -210,7 +210,9 @@ class ComplexFutureAction[T] extends FutureAction[T] {
} catch {
case e: Exception = p.failure(e)
} finally {
-thread = null
+ComplexFutureAction.this.synchronized {
--- End diff --
ok thanks for the explanation. can you add some inline comment there
explaining this ensures we do not reset the thread in the middle of a
cancellation?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60857035 @rxin Done. Is it OK? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60857510 [Test build #22403 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22403/consoleFull) for PR 2957 at commit [`edf0aee`](https://github.com/apache/spark/commit/edf0aee104eb2e9051c370983920ee73e77db56a). * This patch merges cleanly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60863669 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22403/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60863666 [Test build #22403 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22403/consoleFull) for PR 2957 at commit [`edf0aee`](https://github.com/apache/spark/commit/edf0aee104eb2e9051c370983920ee73e77db56a). * This patch **passes all tests**. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
GitHub user zsxwing opened a pull request:
https://github.com/apache/spark/pull/2957
[SPARK-4097] Fix the race condition of 'thread'
There is a chance that `thread` is null when calling `thread.interrupt()`.
```Scala
override def cancel(): Unit = this.synchronized {
_cancelled = true
if (thread != null) {
thread.interrupt()
}
}
```
Should put `thread = null` into a `synchronized` block to fix the race
condition.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zsxwing/spark SPARK-4097
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2957.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2957
commit c5cfeca22b46d6538f669ebbe5dd10fd198583c9
Author: zsxwing zsxw...@gmail.com
Date: 2014-10-27T08:21:32Z
Fix the race condition of 'thread'
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60561668 [Test build #22284 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22284/consoleFull) for PR 2957 at commit [`c5cfeca`](https://github.com/apache/spark/commit/c5cfeca22b46d6538f669ebbe5dd10fd198583c9). * This patch merges cleanly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60563934 You can also just take a local reference to the thread, and operate on it. The local reference will of course be null in both cases or not-null in both cases. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60565342 You can also just take a local reference to the thread, and operate on it. The local reference will of course be null in both cases or not-null in both cases. A local reference may interrupt other tasks in the executor. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60565582 [Test build #22289 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22289/consoleFull) for PR 2957 at commit [`c5cfeca`](https://github.com/apache/spark/commit/c5cfeca22b46d6538f669ebbe5dd10fd198583c9). * This patch merges cleanly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60566474 Test FAILed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22288/ Test FAILed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60568910 [Test build #22284 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22284/consoleFull) for PR 2957 at commit [`c5cfeca`](https://github.com/apache/spark/commit/c5cfeca22b46d6538f669ebbe5dd10fd198583c9). * This patch **passes all tests**. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60568924 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22284/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60572797 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22289/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60572789 [Test build #22289 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22289/consoleFull) for PR 2957 at commit [`c5cfeca`](https://github.com/apache/spark/commit/c5cfeca22b46d6538f669ebbe5dd10fd198583c9). * This patch **passes all tests**. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60574364 @zsxwing You're trying to handle the case where `thread` changes between checking `thread != null` and calling `thread.interrupt()` right? Copying `thread` to a local variable is a simple way to address that. The call to `cancel()` may view `thread` before or after the `finally` block makes it `null` but that's fine either way. Is there a different race condition you're trying to solve? you can't call `run()` from multiple threads here, but this change wouldn't change that. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60577583
The usage of `run` is in `AsyncRDDActions.takeAsync`. It uses
`scala.concurrent.ExecutionContext.Implicits.global` as the implicit
`ExecutionContext`. Because
`scala.concurrent.ExecutionContext.Implicits.global` is a general executor,
there may be other tasks submitted to it.
If using a local variable such as
```Scala
1val t = thread
2if (t != null) {
3 t.interrupt()
4}
```
After checking `t!=null`, assume that this thread T1 is suspended. Then in
thread T2, `thread` becomes `null`, and
`scala.concurrent.ExecutionContext.Implicits.global` starts to run other task
`X`. Then T1 is resumed, and run `t.interrupt()`, it will interrupt task `X`.
I'm trying to avoid it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60578902 How would running another task X change the value of `t` in T1's stack? I understand it could modify `thread`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60579222 How would running another task X change the value of t in T1's stack? No, I mean another task X will observe `t.interrupt()` because task X happens to run in the same thread. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60579828 Ah right. But wasn't this already a potential problem and still after this change? the caller's call to `cancel()` may be just about to happen, when the old task finishes, thread is nulled, thread is set back to the same thread by the new task. `cancel()` would still affect the new task. If that's the race, I think another change would be needed to address it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60580481 No, I checked `ThreadPoolExecutor` here: http://www.grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8-b132/java/util/concurrent/ThreadPoolExecutor.java#1129 The interrupted state will be reset before running a new task. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60580846 In summary, the local variable solution may interrupt the thread during running another new task. So that's a potential problem. Although my solution here may leave the thread interrupted after the old task finishes, `ThreadPoolExecutor` will clear it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60581156 But the call to interrupt the old task interrupt happens after the new task begins on the thread. The prior interrupt state doesn't matter; the thread is interrupted in running the new task. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60581389 But the call to interrupt the old task interrupt happens after the new task begins on the thread. Because I use the same lock, this should not happen. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60581813
There are only two cases:
```Scala
t1:
if (thread != null) {
thread.interrupt()
}
t2:
thread = null
```
Or:
```Scala
t2:
thread = null
t1:
if (thread != null) {
thread.interrupt()
}
```
In the second case, when the new task begins to run, because thread is
already null, `thread.interrupt()` won't be called.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60583305 Yes I get that this is not just about preventing an NPE, which is what I thought the intent was originally. I think this does not go far enough to prevent the problem though. - Task A executes on thread T - thread is set to T - Another thread tries to cancel() A but yields before calling it - Task A completes - thread is null - Task B runs on thread T - thread is set to T - Call to cancel() continues and interrupts B executing on T A local var doesn't help this. Is this the real problem? or will this never happen for another reason? the locks have no effect on this sequence. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60583790 You example assumes `ComplexFutureAction.run` may be called more than once. However after reviewing other codes, I don't think `ComplexFutureAction.run` can be called more than once, for example, it uses the same Promise but Promise cannot completed more than once. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60584045 If `ComplexFutureAction.run` is called only once, I think my current fix is enough. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60584425 Here is the usage of `ComplexFutureAction`: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/rdd/AsyncRDDActions.scala#L66 I think `ComplexFutureAction` assumes `run` is called only once. Is it right? @rxin --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user srowen commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60584591 Yeah I think that would be enough to guarantee it, and it makes sense. Either interrupt happens before the task nulls the reference, in which case the task is not done no other task will have started on the same thread, or, it happens after in which case it will find the reference null and do nothing. +1! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-4097] Fix the race condition of 'thread...
Github user zsxwing commented on the pull request: https://github.com/apache/spark/pull/2957#issuecomment-60584794 @srowen thank you for reviewing it:) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
