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

[Added] natsConnection_Reconnect #756

Closed
wants to merge 6 commits into from
Closed

[Added] natsConnection_Reconnect #756

wants to merge 6 commits into from

Conversation

levb
Copy link
Collaborator

@levb levb commented May 3, 2024

@levb levb requested review from mtmk, Jarema and kozlovic May 3, 2024 15:57
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 68.67%. Comparing base (3f02b1d) to head (27fd5df).
Report is 1 commits behind head on main.

Files Patch % Lines
src/conn.c 57.14% 5 Missing and 10 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #756      +/-   ##
==========================================
- Coverage   68.72%   68.67%   -0.06%     
==========================================
  Files          39       39              
  Lines       15176    15189      +13     
  Branches     3137     3139       +2     
==========================================
+ Hits        10430    10431       +1     
- Misses       1695     1704       +9     
- Partials     3051     3054       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Could you review my comment and see if we could make the changes only in the new function (no existing code would need to be changed)?

{
natsStatus ls = NATS_OK;
Copy link
Member

Choose a reason for hiding this comment

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

There was a reason we used a different status here. What we wanted to report as the error was the error that was given to processOpError(), not an internal one that we may encounter trying to setup the reconnect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right, and I think my code still does that (newErr), but in the case of a natsConnection_Reconnect failing on a to fire off doReconnect we want to return the actual failure, right? (like, out of memory for instance)

if (natsConnection_IsClosed(nc))
return nats_setDefaultError(NATS_INVALID_ARG);

IFOK(s, _forceReconnect(nc, NATS_OK, NULL));
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for the IFOK here since there is no change of s in lines above.

But overall, I think these changes are way more complicated than it needs to be (or I am missing something). If we want to cause a reconnect, locking the connection and and if the socket is valid, closing it should be enough, no?

Of course, verification that the connection is set in such a way that it allows for reconnection (I don't recall if there are options that would totally prevent a reconnect) before doing so, and if not, returning an error indication that reconnect() failed because the connection would not reconnect because of options setting, would be best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

way more complicated than it needs to be (or I am missing something). If we want to cause a reconnect, locking the connection and and if the socket is valid, closing it should be enough, no?

@kozlovic you're likely right, I was modeling after the go changes that call an existing re-connect method, and I figured this to be the closest. I'll try what you suggested, in a separate branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #757

@levb levb closed this May 3, 2024
@levb levb deleted the lev-forced-reconnect branch June 13, 2024 06:45
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