-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix: no DeadLetter
s when publishing to a Topic
with no subscribers
#5561
Conversation
This relies on the Topic ActorRef termination and if we want it to publish to deadletters when there are zero subscribers, we will need to change the current design: akka.net/src/contrib/cluster/Akka.Cluster.Tools/PublishSubscribe/DistributedPubSubMediator.cs Lines 497 to 529 in dbe4999
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments on the structure of the test
src/contrib/cluster/Akka.Cluster.Tools.Tests/PublishSubscribe/DistributedPubSubMediatorSpec.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools.Tests/PublishSubscribe/DistributedPubSubMediatorSpec.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools.Tests/PublishSubscribe/DistributedPubSubMediatorSpec.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools.Tests/PublishSubscribe/DistributedPubSubMediatorSpec.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aaronontheweb improved the test with the latest commit
src/contrib/cluster/Akka.Cluster.Tools.Tests/PublishSubscribe/DistributedPubSubMediatorSpec.cs
Outdated
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Tools.Tests/PublishSubscribe/DistributedPubSubMediatorSpec.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments on the changes included in here
_ = ExpectMsg<UnsubscribeAck>(); | ||
|
||
// assert | ||
await EventFilter.DeadLetter<object>().ExpectAsync(1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reproduction - as @eaba pointed out this spec would fail before because the TTL value kept the topic actor alive for 120s. The real bug was that the topic actor never published any DeadLetter
instances if it was alive but had no subscribers.
@@ -643,7 +643,7 @@ private long NextVersion() | |||
|
|||
private IActorRef NewTopicActor(string encodedTopic) | |||
{ | |||
var t = Context.ActorOf(Actor.Props.Create(() => new Topic(_settings.RemovedTimeToLive, _settings.RoutingLogic)), encodedTopic); | |||
var t = Context.ActorOf(Actor.Props.Create(() => new Topic(_settings.RemovedTimeToLive, _settings.RoutingLogic, _settings.SendToDeadLettersWhenNoSubscribers)), encodedTopic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Propagate akka.cluster.pub-sub.send-to-dead-letters-when-no-subscribers
to the topic actors, who are the ones who actually need to implement this.
/// A <see cref="DeadLetter"/> published when there are no subscribers | ||
/// for a topic that has received a <see cref="Publish"/> event. | ||
/// </summary> | ||
internal readonly struct NoSubscribersDeadLetter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an envelope NoSubscribersDeadLetter
in order to help make it clear inside the user's logging system why this DeadLetter
was published. Created it as a readonly struct
because it will immediately be captured inside a DeadLetter
and therefore we can save on an allocation here in the event of a large number of deadletters being published all at once.
/// <param name="emptyTimeToLive">The TTL for how often this actor will be removed.</param> | ||
/// <param name="sendToDeadLettersWhenNone">When set to <c>true</c>, this actor will | ||
/// publish a <see cref="DeadLetter"/> for each message if the total number of subscribers == 0.</param> | ||
protected TopicLike(TimeSpan emptyTimeToLive, bool sendToDeadLettersWhenNone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated constructor to receive new value. These are all internal
classes so no breaking changes here.
@@ -116,22 +151,23 @@ protected bool DefaultReceive(object message) | |||
default: | |||
foreach (var subscriber in Subscribers) | |||
subscriber.Forward(message); | |||
|
|||
// no subscribers | |||
if (Subscribers.Count == 0 && SendToDeadLettersWhenNoSubscribers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bugfix: @eaba pointed out that this actor never was programmed to publish to deadletters when the subscriber count is zero, so we do this after the foreach
loop (arbitrary) if we're configured for it. This causes the reproduction spec to now pass.
DeadLetter
s when publishing to a Topic
with no subscribers
The summary of the PubSub design is this, as it relates to #5352, if there are no subscribers, wait on the Deadline and if no new subscriber within that window terminate the Topic actor.
It is the
waiting on the Deadline
that is the actual issue.