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.

Sign up for GitHub

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

Jump to bottom

[FIXED] Closing connection on max subscriptions exceeded #1709

Merged
merged 1 commit into from
Oct 18, 2024
Merged

[FIXED] Closing connection on max subscriptions exceeded #1709

merged 1 commit into from
Oct 18, 2024

Conversation

piotrpio
Copy link
Collaborator

@piotrpio piotrpio commented Sep 2, 2024

Signed-off-by: Piotr Piotrowski piotr@synadia.com

@piotrpio piotrpio requested a review from Jarema September 2, 2024 21:21
@piotrpio piotrpio force-pushed the server-err-handling branch from 2f9f9db to efa0f6b Compare September 5, 2024 09:12
@coveralls
Copy link

coveralls commented Sep 5, 2024 •
edited
Loading

Coverage Status

coverage: 84.855% (-0.006%) from 84.861%
when pulling aff4d89 on server-err-handling
into c7cf345 on main.

@wallyqs
Copy link
Member

wallyqs commented Sep 5, 2024

any other side effect from the changes? seems this also now avoids closing when creating an invalid subscription?

@derekcollison
Copy link
Member

Just checking, when we get this error we should not be closing the connection from the client side.

@piotrpio
Copy link
Collaborator Author

@derekcollison correct, this used to be the case but this change will cause it to just invoke error callback.

wallyqs
wallyqs approved these changes Oct 7, 2024
View reviewed changes
Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

@fmontorsi-equinix
Copy link
Contributor

hey @piotrpio ,
I actually stumbled on this PR and I wonder if this might fix the behavior I described at nats-io/nats-server#5994 . In particular I'm trying to understand why my consumers are stopping processing shortly after receiving the "nats: Exceeded MaxWaiting" error...
Do you think this PR would restore processing in the consumers that received that "nats: Exceeded MaxWaiting" error?
thanks

@Jarema
Copy link
Member

Jarema commented Oct 14, 2024

This one is about subscriptions, not max_waiting in pull requests, so I doubt its related.

@piotrpio
[FIXED] Closing connection on max subscriptions exceeded
aff4d89 
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio force-pushed the server-err-handling branch from 5a7192a to aff4d89 Compare October 18, 2024 09:58
Jarema
Jarema approved these changes Oct 18, 2024
View reviewed changes
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

@piotrpio piotrpio merged commit ee22dc4 into main Oct 18, 2024
6 checks passed
@piotrpio piotrpio deleted the server-err-handling branch October 18, 2024 10:54
piotrpio added a commit that referenced this pull request Dec 13, 2024
@piotrpio
[FIXED] Closing connection on max subscriptions exceeded (#1709)
08ae14b 
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Dec 13, 2024
@piotrpio
[FIXED] Closing connection on max subscriptions exceeded (#1709)
240fe36 
Signed-off-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jarema Jarema Jarema approved these changes

@wallyqs wallyqs wallyqs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@piotrpio @coveralls @wallyqs @derekcollison @fmontorsi-equinix @Jarema