Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforcing ordering of Terminated wrt remote/local #2835 #997

Merged
merged 2 commits into from Jan 4, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions akka-actor/src/main/scala/akka/actor/Address.scala
Expand Up @@ -19,10 +19,27 @@ import scala.collection.immutable
*/ */
@SerialVersionUID(1L) @SerialVersionUID(1L)
final case class Address private (protocol: String, system: String, host: Option[String], port: Option[Int]) { final case class Address private (protocol: String, system: String, host: Option[String], port: Option[Int]) {
// Please note that local/non-local distinction must be preserved:
// host.isDefined == hasGlobalScope
// host.isEmpty == hasLocalScope
// hasLocalScope == !hasGlobalScope


def this(protocol: String, system: String) = this(protocol, system, None, None) def this(protocol: String, system: String) = this(protocol, system, None, None)
def this(protocol: String, system: String, host: String, port: Int) = this(protocol, system, Option(host), Some(port)) def this(protocol: String, system: String, host: String, port: Int) = this(protocol, system, Option(host), Some(port))


/**
* Returns true if this Address is only defined locally. It is not safe to send locally scoped addresses to remote
* hosts. See also [[akka.actor.Address#hasGlobalScope]].
*/
def hasLocalScope: Boolean = host.isEmpty

/**
* Returns true if this Address is usable globally. Unlike locally defined addresses ([[akka.actor.Address#hasLocalScope]])
* addresses of global scope are safe to sent to other hosts, as they globally and uniquely identify an addressable
* entity.
*/
def hasGlobalScope: Boolean = host.isDefined

/** /**
* Returns the canonical String representation of this Address formatted as: * Returns the canonical String representation of this Address formatted as:
* *
Expand Down
26 changes: 19 additions & 7 deletions akka-actor/src/main/scala/akka/actor/dungeon/DeathWatch.scala
Expand Up @@ -4,7 +4,7 @@


package akka.actor.dungeon package akka.actor.dungeon


import akka.actor.{ Terminated, InternalActorRef, ActorRef, ActorCell, Actor, Address, AddressTerminated } import akka.actor.{ Terminated, InternalActorRef, ActorRef, ActorRefScope, ActorCell, Actor, Address, AddressTerminated }
import akka.dispatch.{ Watch, Unwatch } import akka.dispatch.{ Watch, Unwatch }
import akka.event.Logging.{ Warning, Error, Debug } import akka.event.Logging.{ Warning, Error, Debug }
import scala.util.control.NonFatal import scala.util.control.NonFatal
Expand Down Expand Up @@ -51,12 +51,24 @@ private[akka] trait DeathWatch { this: ActorCell ⇒
if (!watchedBy.isEmpty) { if (!watchedBy.isEmpty) {
val terminated = Terminated(self)(existenceConfirmed = true, addressTerminated = false) val terminated = Terminated(self)(existenceConfirmed = true, addressTerminated = false)
try { try {
watchedBy foreach { def sendTerminated(ifLocal: Boolean)(watcher: ActorRef): Unit =
watcher ⇒ if (watcher.asInstanceOf[ActorRefScope].isLocal == ifLocal) watcher.tell(terminated, self)
try watcher.tell(terminated, self) catch {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have the try-catch around tell?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intuition would be that not all ActorRef's are actual actors, so we might get exceptions when invoking tell. But I am not a dungeon-dweller :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the try-catch is an artifact from the past where tell was allowed to throw exceptions. However, if it does throw an exception, if we don't catch and continue, the system is broken.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we rely on that tell should not throw exception in other places, to avoid sprinkling this all over the place

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, let's remove and pray :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could sendSystemMessage throw an exception? Because there is a similar catch construct just one method down.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not likely, but lets open a ticket about fixing all such occurences separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

case NonFatal(t) ⇒ publish(Error(t, self.path.toString, clazz(actor), "deathwatch")) /*
} * It is important to notify the remote watchers first, otherwise RemoteDaemon might shut down, causing
} * the remoting to shut down as well. At this point Terminated messages to remote watchers are no longer
* deliverable.
*
* The problematic case is:
* 1. Terminated is sent to RemoteDaemon
* 1a. RemoteDaemon is fast enough to notify the terminator actor in RemoteActorRefProvider
* 1b. The terminator is fast enough to enqueue the shutdown command in the remoting
* 2. Only at this point is the Terminated (to be sent remotely) enqueued in the mailbox of remoting
*
* If the remote watchers are notified first, then the mailbox of the Remoting will guarantee the correct order.
*/
watchedBy foreach sendTerminated(ifLocal = false)
watchedBy foreach sendTerminated(ifLocal = true)
} finally watchedBy = ActorCell.emptyActorRefSet } finally watchedBy = ActorCell.emptyActorRefSet
} }
} }
Expand Down
Expand Up @@ -190,7 +190,11 @@ private[akka] trait FaultHandling { this: ActorCell ⇒


private def finishTerminate() { private def finishTerminate() {
val a = actor val a = actor
// The following order is crucial for things to work properly. Only chnage this if you're very confident and lucky. /* The following order is crucial for things to work properly. Only change this if you're very confident and lucky.
*
* Please note that if a parent is also a watcher then ChildTerminated and Terminated must be processed in this
* specific order.
*/
try if (a ne null) a.postStop() try if (a ne null) a.postStop()
finally try dispatcher.detach(this) finally try dispatcher.detach(this)
finally try parent.sendSystemMessage(ChildTerminated(self)) finally try parent.sendSystemMessage(ChildTerminated(self))
Expand Down