Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Switching to a more streamlined approach for systemInvoke error handling... #422

Merged
merged 3 commits into from Apr 27, 2012

Conversation

Projects
None yet
3 participants
Owner

viktorklang commented Apr 25, 2012

..., resembling the approach used by invoke

WARNING! WARNING! WARNING! WARNING! WARNING! WARNING!

Delicate change ahead

@rkuhn rkuhn and 1 other commented on an outdated diff Apr 25, 2012

akka-actor/src/main/scala/akka/actor/ActorCell.scala
dispatcher.reportFailure(new LogEventException(Error(e, self.path.toString, clazz(actor), "error while processing " + message), e))
- throw e
+ // prevent any further messages to be processed until the actor has been restarted
+ dispatcher.suspend(this)
+ if (actor ne null) actor.supervisorStrategy.handleSupervisorFailing(self, children)
+ parent.tell(Failed(if (e.isInstanceOf[InterruptedException]) ActorInterruptedException(e) else e), self) // Wrap InterruptedExceptions
@rkuhn

rkuhn Apr 25, 2012

Collaborator

this should be in finally block as it was before, because handleSupervisorFailing is not trusted code (potentially)

@rkuhn rkuhn and 1 other commented on an outdated diff Apr 25, 2012

akka-actor/src/main/scala/akka/actor/ActorCell.scala
dispatcher.reportFailure(new LogEventException(Error(e, self.path.toString, clazz(actor), "error while processing " + message), e))
- throw e
+ // prevent any further messages to be processed until the actor has been restarted
+ dispatcher.suspend(this)
+ if (actor ne null) actor.supervisorStrategy.handleSupervisorFailing(self, children)
+ parent.tell(Failed(if (e.isInstanceOf[InterruptedException]) ActorInterruptedException(e) else e), self) // Wrap InterruptedExceptions
+ throw e // FIXME should we rethrow?
@rkuhn

rkuhn Apr 25, 2012

Collaborator

I don’t think we should, let’s have a closer look tomorrow

@viktorklang

viktorklang Apr 26, 2012

Owner

only throws InterruptedException or Fatal now

@rkuhn rkuhn and 2 others commented on an outdated diff Apr 25, 2012

akka-actor/src/main/scala/akka/util/NonFatal.scala
+ private def isFatal(t: Throwable): Boolean =
+ (t.isInstanceOf[VirtualMachineError] && !t.isInstanceOf[StackOverflowError]) || t.isInstanceOf[ThreadDeath] || t.isInstanceOf[InterruptedException] || t.isInstanceOf[LinkageError]
@rkuhn

rkuhn Apr 25, 2012

Collaborator

line breaks, please.

@patriknw

patriknw Apr 26, 2012

Owner

I think this is harder to read than the original.
What is the advantage?

@viktorklang

viktorklang Apr 26, 2012

Owner

This is a fair point Patrik, I had originally written the match in systemInvoke to use the new isFatal method, but abandoned that when Roland suggested the cleaner extractor match. I'll revert my change. (Although it's faster ;-) )

@rkuhn rkuhn commented on an outdated diff Apr 25, 2012

akka-actor/src/main/scala/akka/actor/ActorCell.scala
}
+ currentMessage = null // reset current message after successful invocation
+ } catch {
+ case e @ (_: InterruptedException | NonFatal(_))
+ dispatcher.reportFailure(new LogEventException(Error(e, self.path.toString, clazz(actor), e.getMessage), e))
+ dispatcher.suspend(this) // prevent any further messages to be processed until the actor has been restarted
+ if (actor ne null) actor.supervisorStrategy.handleSupervisorFailing(self, children)
+ parent.tell(Failed(if (e.isInstanceOf[InterruptedException]) ActorInterruptedException(e) else e), self) // Wrap InterruptedExceptions
@rkuhn

rkuhn Apr 25, 2012

Collaborator

what about re-throwing InterruptedException?

@rkuhn

rkuhn Apr 27, 2012

Collaborator

this needs the same treatment as the corresponding catch in systemInvoke.

Collaborator

rkuhn commented Apr 25, 2012

this is delicate stuff, we should talk it through tomorrow

viktorklang added some commits Apr 26, 2012

@viktorklang viktorklang Reverting to the old NonFatal impl since the new wasn't needed and ad…
…ding finally block and escalation only in the case of InterruptedException or fatal
f56dee1
@viktorklang viktorklang Cleaning up and streamlining the error handling of the invoke methods…
… as suggested by Roland
20866be

@viktorklang viktorklang added a commit that referenced this pull request Apr 27, 2012

@viktorklang viktorklang Merge pull request #422 from akka/wip-1880-revise-error-handling-in-s…
…ystem-invoke-√

Switching to a more streamlined approach for systemInvoke error handling...
855b88b

@viktorklang viktorklang merged commit 855b88b into master Apr 27, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment