Skip to content
New issue

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

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

Already on GitHub? # to your account

=receptionist Fix receptionist crash on concurrent registrations; those are fine and expected #1059

Merged
merged 6 commits into from
Aug 30, 2022

Conversation

ktoso
Copy link
Member

@ktoso ktoso commented Aug 30, 2022

New receptionist was too aggressive in what it deemed acceptable to be send onNext to the reception stream.

Concurrent messages are fine; specifically if [first:1] then [first:1, second:1] happens, these may have been concurrent in vector clock meaning, but it means that the event has more information so we should be emitting it

Actual fix is cc51dba (#1059) and the rest is supporting infra

resolves #1053

@ktoso ktoso requested a review from yim-lee August 30, 2022 10:26
@ktoso
Copy link
Member Author

ktoso commented Aug 30, 2022

Joining timeout in

19:32:33 Test Case 'ClusterSingletonPluginClusteredTests.test_remoteCallShouldFailAfterAllocationTimedOut' started at 2022-08-30 10:32:20.538
19:32:33 <EXPR>:0: error: ClusterSingletonPluginClusteredTests.test_remoteCallShouldFailAfterAllocationTimedOut : threw error "
19:32:33         try await self.assertMemberStatus(on: second, node: firstNode, is: .down, within: .seconds(10))
19:32:33                              ^~~~~~
19:32:33 error: MembershipError(awaitStatusTimedOut(10.0 seconds, Optional(MembershipError(statusRequirementNotMet(expected: DistributedActors.Cluster.MemberStatus.down, found: Member(sact://first:1722502985102675705@127.0.0.1:7111, status: joining, reachability: unreachable)), at: DistributedActors/ClusterControl.swift:252))), at: DistributedActors/ClusterControl.swift:338)"

@ktoso
Copy link
Member Author

ktoso commented Aug 30, 2022

Seems to be same as #1034 - the nodes seem to sometimes need longer to join up for the test.

@ktoso
Copy link
Member Author

ktoso commented Aug 30, 2022

Integration test is green which is good, this one would have triggered the bug this is fixing :-)

@ktoso ktoso changed the title =multi-node improve printouts to always be one-by-one (logs) =receptionist Fix receptionist crash on concurrent registrations; those are fine and expected Aug 30, 2022
Copy link
Member

@yim-lee yim-lee left a comment

Choose a reason for hiding this comment

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

One typo but otherwise LGTM 👍

}
let key = DistributedReception.Key(Guest.self, id: keyID)

await self.whenLocal { myself in
Copy link
Member

Choose a reason for hiding this comment

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

This reads very nicely 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I recently realized we don't always have to do the __ trickery by the way :)

@@ -429,22 +434,18 @@ internal final class AnyDistributedReceptionListingSubscription: Hashable, @unch
// the seen vector was unaffected by the merge, which means that the
// incoming registration version was already seen, and thus we don't need to emit it again
return false
case .happenedAfter:
case .happenedAfter, .concurrent:
Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense I think.

…butedReceptionist.swift

Co-authored-by: Yim Lee <yim_lee@apple.com>
@ktoso ktoso merged commit 24ae7c2 into apple:main Aug 30, 2022
@ktoso ktoso deleted the wip-receptionist-gossip-back-in-time branch August 30, 2022 23:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application crashes with fatal error when I subscribe to receptionist.listing updates
2 participants