Github user squito commented on the pull request:
https://github.com/apache/spark/pull/6205#issuecomment-107765371
Hi @BryanCutler,
I finally took a closer look at this. I hadn't realized until you brought
it up that even `ask` takes one timeout on the original call, and then another
one in `Await.result`. If the timeout to the original `ask` expires, you get
an `AskTimeoutException`, eg. `akka.pattern.AskTimeoutException: Ask timed out
on [Actor[akka://EchoSystem/user/echoA#-553815755]] after [50 ms]`, but if its
the timeout in `Await.result`, you just get a `TimeoutException`, eg.
`java.util.concurrent.TimeoutException: Futures timed out after [50
milliseconds]`. Eg:
```
val fut = echoActor.ask("hello")(50 millis)
val inBetweenWait = (15 millis)
Thread.sleep(inBetweenWait.toMillis)
val result = Await.result(fut, 50 millis)
```
for me the behavior seems flip flop somewhere around `inBetweenWait = 15
millis`, but that will most likely vary. Even if the `ask` timeout is shorter
than the `Await` timeout, I still might get the exception from `Await`.
In any case, the reason I'm bringing this up is that we're sort of in the
same scenario with `actorSelection.resolveOne` (looks like that timeout is just
[passed on to
`ask`](https://github.com/akka/akka/blob/1dac4010990e6a0790dedceece83b1e5864c37e7/akka-actor/src/main/scala/akka/actor/ActorSelection.scala#L60)).
Your suggestion of using `recover` works for handling that exception from
`ask` or `resolveOne` etc, basically to handle a timeout that might already be
baked into a `Future`. I'd like to keep around what you already have, since
(a) we still need something else for `Await.result` b/c that might timeout
first (as you've noted) and (b) its a lot simpler to understand in my opinion.
I think its worth adding your suggestion as well for those remaining cases
-- its a nice trick :). I didn't see any existing uses for those methods, but
most likely the resulting `Future`s will just get wrapped in an `Await.result`
with the same timeout, but it would still be nice to have that in place. I
think just (a) throwing your own subclass of `TimeoutException` to avoid double
appending and (b) having `timeout.addMessageIfTimeout(future)` (or whatever you
want to call it) take in an `ExecutionContext` seems fine. (You can see that
`scala.concurrent.ExecutionContext.Implicits.global` is imported elsewhere in
`AkkaRpcEnv`.)
phew that was a mouthful. hope this all makes sense
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
