=fix #614 fix deathwatch parent/child guard, ensure single terminated; fix interceptor test #763

ktoso · 2020-09-10T08:46:59Z

Motivation:

Fix flaky test in FAILED: test_interceptor_shouldInterceptSignals #614
the guard protecting "parent needs not watch child explicitly because child always reports termination back" in DeathWatch was in the wrong function -- fixed that
we double notified a parent with Termianted and ChildTerminated -- this is technically harmless but wrong (!), so added a test to notice and fix this as well (it's a result of the above wrong guard).

Result:

Resolves FAILED: test_interceptor_shouldInterceptSignals #614
more correct terminated signals for parent/child (always ONLY ChildTerminated)

…; fix interceptor test via this

ktoso · 2020-09-10T08:51:49Z

Sources/DistributedActors/ActorShell.swift

        with terminationMessage: Message? = nil,
        file: String = #file, line: UInt = #line
    ) -> Watchee where Watchee: DeathWatchable {
-        self.deathWatch.watch(watchee: watchee, with: terminationMessage, myself: self.myself, parent: self._parent, file: file, line: line)


the parent is super unnecessary for ISSUING a watch;

we don't care about OUR parent, only if we are the parent of the being watched actor, as such we can pass myself (self in order to have _children to check them inside there)

it optionally necessary for HANDLING a watch.

it's not required, but we're being defensive now, even if someone sent us such system message we'd do the right thing

ktoso · 2020-09-10T08:51:58Z

Sources/DistributedActors/Behaviors.swift

                    try postStop(context)
                }
-                return .same // will be ignored
+                return .stop // will be ignored


just cosmetic tbh

ktoso · 2020-09-10T08:52:09Z

Sources/DistributedActors/DeathWatch.swift


    /// Performed by the sending side of "watch", therefore the `watcher` should equal `context.myself`
-    public mutating func watch<Watchee>(
+    mutating func watch<Watchee>(


does not have to be public

ktoso · 2020-09-10T08:52:31Z

Sources/DistributedActors/DeathWatch.swift

            return
        }

-        guard addressableWatchee.address != parent.address else {


whooop! this is in the wrong function, moved to becomeWatchedBy

ktoso · 2020-09-10T08:52:57Z

Sources/DistributedActors/DeathWatch.swift

+            // not yet watching, so let's add it:
            self.watching[addressableWatchee] = OnTerminationMessage(customize: terminationMessage)

+            if !watcher.children.contains(identifiedBy: watchee.asAddressable.address) {


we proactively avoid sending a message which would have been ignored anyway

ktoso · 2020-09-10T08:53:24Z

Sources/DistributedActorsTestKit/TestProbes.swift

+    /// and our direct message arrives first, before the watch at the destination, causing potentially confusing behavior
+    /// in some very ordering delicate testing scenarios.
+    public func forward<Message>(_ message: Message, to target: ActorRef<Message>, file: String = #file, line: UInt = #line) where Message: Codable {
+        self.internalRef.tell(ProbeCommands.forwardCommand(send: { () in target.tell(message, file: file, line: line) }))


not really necessary here, since watching a dead actor works as well, but I wanted to make sure to see the exact ordering I'm testing.

ktoso · 2020-09-10T08:55:10Z

Tests/DistributedActorsTests/DeathWatchTests.swift

+        terminated.address.path.shouldEqual(try! ActorPath._user.appending("parent").appending("stopper"))
+        terminated.existenceConfirmed.shouldBeTrue()
+        terminated.nodeTerminated.shouldBeFalse()
+        terminated.shouldBe(Signals.ChildTerminated.self)


which would have potentially failed and may have received Terminated before (!)

(or rather, the expect no message would have gotten it)

ktoso · 2020-09-10T08:55:31Z

Tests/DistributedActorsTests/InterceptorTests.swift

        let terminated = try p.expectMessage()
        (terminated.address.name == "stopperOne" || terminated.address.name == "stopperTwo").shouldBeTrue()
-        try p.expectNoMessage(for: .milliseconds(100))
+        try p.expectNoMessage(for: .milliseconds(500))


to be extra sure

ktoso · 2020-09-10T08:55:37Z

Tests/DistributedActorsTests/InterceptorTests.swift

    }

-    func test_interceptor_shouldRemainWHenReturningStoppingWithPostStop() throws {
+    func test_interceptor_shouldRemainWhenReturningStoppingWithPostStop() throws {


yim-lee

Great find!

yim-lee · 2020-09-10T16:20:15Z

Sources/DistributedActors/DeathWatch.swift

            return
        }

-        guard addressableWatchee.address != parent.address else {


ktoso added 2 commits September 10, 2020 17:43

=fix #614 fix deathwatch parent/child guard, ensure single terminated…

f1db406

…; fix interceptor test via this

a bit smaller timeout is enough

b5212b6

ktoso commented Sep 10, 2020

View reviewed changes

ktoso requested a review from yim-lee September 10, 2020 08:55

yim-lee approved these changes Sep 10, 2020

View reviewed changes

Sources/DistributedActors/DeathWatch.swift

return

}

guard addressableWatchee.address != parent.address else {

Copy link

Member

yim-lee Sep 10, 2020

Choose a reason for hiding this comment

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

🙀

ktoso merged commit 5f829e3 into apple:main Sep 11, 2020

ktoso deleted the wip-interceptor-test branch September 11, 2020 00:52

=fix #614 fix deathwatch parent/child guard, ensure single terminated; fix interceptor test #763

=fix #614 fix deathwatch parent/child guard, ensure single terminated; fix interceptor test #763

Uh oh!

Conversation

ktoso commented Sep 10, 2020

Motivation:

Result:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yim-lee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants