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

Only retain non-closed subscriptions on reconnect #454

Merged

Conversation

caspervonb
Copy link
Collaborator

If a subscriber is dropped during reconnect, it likely it never fired the unsubscribe command. But we know it closed so we can just not ever re-create the subscription.

async-nats/src/lib.rs Outdated Show resolved Hide resolved
@caspervonb caspervonb marked this pull request as ready for review May 26, 2022 17:08
@caspervonb caspervonb requested a review from Jarema May 26, 2022 17:08
@Jarema
Copy link
Member

Jarema commented May 26, 2022

Are you sure?
We first remove the subscription, then try to talk to the socket.
Should be dropped, right?

@Jarema
Copy link
Member

Jarema commented May 26, 2022

                if let Some(subscription) = self.subscriptions.get_mut(&sid) {
                    subscription.max = max;
                    match subscription.max {
                        Some(n) => {
                            if subscription.delivered >= n {
                                self.subscriptions.remove(&sid);
                            }
                        }
                        None => {
                            self.subscriptions.remove(&sid);
                        }
                    }

                    if let Err(err) = self
                        .connection
                        .write_op(ClientOp::Unsubscribe { sid, max })
                        .await
                    {
                        println!("Send failed with {:?}", err);
                    }
                }

or I'm tired and missing the point? 😄

@caspervonb
Copy link
Collaborator Author

caspervonb commented May 26, 2022

Are you sure?
We first remove the subscription, then try to talk to the socket.
Should be dropped, right?

Some cases where it will fail, e.g if we're reconnecting when its being dropped then the command can just never be dispatched if the command buffer is full.

That's fine, if we just don't do the initial re-subscription.

or I'm tired and missing the point?

Short circuit, when reconnecting, there's no point in re-subscribing to a subscription which has no receiver as it would go no-where. We can just not subscribe as we can easily check and it's a safe spot to do so.

Otherwise a sub on reconnect would be:

SUB 1
MSG 1
UNSUB 1

TL,DR; drop isn't infallible, but there is no reason to re-subscribe causing a chain of 3 protocol messages to a thing we know is already dropped.

@caspervonb caspervonb force-pushed the ignore-closed-subscriptons-on-reconnect branch 2 times, most recently from 4f1a8d0 to 9e5e4f4 Compare May 27, 2022 06:58
@caspervonb caspervonb changed the title Ignore closed subscriptions on reconnect Only retain non-closed subscriptions on reconnect May 27, 2022
@caspervonb caspervonb force-pushed the ignore-closed-subscriptons-on-reconnect branch from 9e5e4f4 to feb2186 Compare May 27, 2022 09:04
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@caspervonb caspervonb merged commit 759fe60 into nats-io:main May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants