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] Conn and Subscription Close()/Unubscribe() could not be retried #360

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

kozlovic
Copy link
Member

If a connection is closed, or a subscription is closed or unsubscribed,
and an error occurs at the protocol level, for instance getting a
timeout or the NATS connection is currently disconnected, etc..
the Close()/Unsubscribe() calls would return an error but the user
would not be able to invoke them again, in the sense that these
calls would bail out early because those objects were already marked
as closed.

This was problematic because if an application closes a connection for
instance but gets a timeout, then there is no way for the application
to really try to tell the server that the connection should be closed.
(same for subscription).

This PR let the user call Close()/Unsubscribe() if the library failed
to get a response from the server (likely timeout or NATS communication
issue). However, if the server returns an error, then the connection/
subscription will be considered fully closed and user won't be able
to call the close API again. The alternative would be to set the
object as fully closed only in case of total success...

Signed-off-by: Ivan Kozlovic [email protected]

If a connection is closed, or a subscription is closed or unsubscribed,
and an error occurs at the protocol level, for instance getting a
timeout or the NATS connection is currently disconnected, etc..
the Close()/Unsubscribe() calls would return an error but the user
would not be able to invoke them again, in the sense that these
calls would bail out early because those objects were already marked
as closed.

This was problematic because if an application closes a connection for
instance but gets a timeout, then there is no way for the application
to really try to tell the server that the connection should be closed.
(same for subscription).

This PR let the user call Close()/Unsubscribe() if the library failed
to get a response from the server (likely timeout or NATS communication
issue). However, if the server returns an error, then the connection/
subscription will be considered fully closed and user won't be able
to call the close API again. The alternative would be to set the
object as fully closed only in case of total success...

Signed-off-by: Ivan Kozlovic <[email protected]>
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 92.875% when pulling df0a9c3 on close_retry_on_err into 9a6e4f2 on main.

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

@kozlovic kozlovic merged commit 93879c7 into main Nov 9, 2021
@kozlovic kozlovic deleted the close_retry_on_err branch November 9, 2021 21:49
kozlovic added a commit that referenced this pull request Nov 17, 2021
In PR #360, we changed the default behavior of connection Close()
in that if the library failed to sent the close protocol or received
its response, the Close() could be retried. In order to do that,
the underlying NATS connection would not be closed.

This has side-effects in that the NATS connection would be left
opened or possibly reconnect when an user would have previously
called Close(), not check error, and expect NATS connection to
be closed.

The introduction of this option allow the default behavior to
remain the same, and users that do want to be able to call Close()
again on a failed call will need to set this option when creating
the connection.

Signed-off-by: Ivan Kozlovic <[email protected]>
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.

3 participants