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

[FIXED] Subscription interest could remain after slow consumer error #5754

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Aug 5, 2024

When a slow consumer error is detected, the server will close the connection. In some race situations, it could be that the subscription interest is left intact, which would cause routed servers to continue routing messages although the interest should have been gone.

The subscription could still be seen in the monitoring subscriptions list (when displaying with details), referencing a client connection that would be in the closed connection state.
The monitoring route page of the other servers would still indicate that there is interest for this subject on the origin server.

Resolves #5539

Signed-off-by: Ivan Kozlovic ivan@synadia.com

When a slow consumer error is detected, the server will close the
connection. In some race situations, it could be that the subscription
interest is left intact, which would cause routed servers to continue
routing messages although the interest should have been gone.

The subscription could still be seen in the monitoring subscriptions
list (when displaying with details), referencing a client connection
that would be in the closed connection state.
The monitoring route page of the other servers would still indicate
that there is interest for this subject on the origin server.

Resolves #5539

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic kozlovic requested a review from a team as a code owner August 5, 2024 21:19
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

In general LGTM - question about avoiding a copy.

server/client.go Outdated Show resolved Hide resolved
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM, good spot!

This avoids the alloc and copy and still allows the connection
to be properly released.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@derekcollison derekcollison self-requested a review August 6, 2024 00:08
@@ -5511,6 +5513,14 @@ func (c *client) closeConnection(reason ClosedState) {
}
}

// Now that we are done with subscriptions, clear the field so that the
// connection can be released and gc'ed.
if kind == CLIENT || kind == LEAF {
Copy link
Member

Choose a reason for hiding this comment

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

FYI when we set this to nil in saveClosedClient it was a much broader set of connection types. Do we think this might cause any GC issues on non client/leaf connections?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because we invoked saveClosedClient() only for CLIENT and LEAF:

if c.kind == CLIENT || c.kind == LEAF {

Copy link
Member

Choose a reason for hiding this comment

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

Sorry was just looking at the internal if..

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 02bd284 into main Aug 6, 2024
5 checks passed
@derekcollison derekcollison deleted the fix_leaked_routed_sub branch August 6, 2024 00:10
neilalexander added a commit that referenced this pull request Aug 6, 2024
Includes the following:

- #5690
- #5725
- #5729
- #5734
- #5735
- #5736
- #5743
- #5744
- #5751
- #5755
- #5754
- #5732
- #5750
- #5756

The following were reverted before this PR:
- #5602

Signed-off-by: Neil Twigg <neil@nats.io>
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.

Incoming messages to the host without connections
3 participants