[Added] natsConnection_Reconnect

[levb]

Simplified version of #756

[codecov]

Codecov Report

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

Project coverage is 68.75%. Comparing base (3f02b1d) to head (5884592).
Report is 1 commits behind head on main.

❗ Current head 5884592 differs from pull request most recent head e85d0b7. Consider uploading reports for the commit e85d0b7 to get more accurate results

Files	Patch %	Lines
src/conn.c	76.92%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #757      +/-   ##
==========================================
+ Coverage   68.72%   68.75%   +0.02%     
==========================================
  Files          39       39              
  Lines       15176    15189      +13     
  Branches     3137     3140       +3     
==========================================
+ Hits        10430    10443      +13     
+ Misses       1695     1692       -3     
- Partials     3051     3054       +3     

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

[kozlovic]

Are you sure we need a change here? Meaning, if you were to just close the socket (not set the fd to invalid, just call natsSock_Close() in the new reconnect call, this should trigger the normal reconnect process. I think there should be no need to change anything else. What made you need to change code here?

[levb]

The go client disregards/overrides its equivalent of nc->opts->allowReconnect when Disconnect() is invoked, I posed a question about it in nats-io/nats.go#1624 (comment).

If we were to error out when not nc->opts->allowReconnect - no changes would be needed, otherwise I need to override it (present implementation).

[kozlovic]

In my view, we should return an error in natsConnection_Reconnect() if the user explicitly disallowed reconnect with the natsOptions_SetAllowReconnect(opts, false). It does not make sense otherwise.

[levb]

Ok, but that also means that the user would not be able to reconnect only "manually", the connection will auto-reconnect on all relevant errors.

I'll make the change now.

[kozlovic]

Sorry I don't understand. The original check here says to check for the current connection status so that we don't attempt to reconnect if we are in the middle of it. The check for !initialConnect was likely introduced when adding the ability to connect asynchronously (that is, call natsConnection_Create() (or Connect) while a server is not running does not fail, but attempt the reconnect right away.

[levb]

I just meant that one won't be able to make a connection with allowReconnect==false if so desired, but then forcefully Reconnect() on it (say if they want to re-auth?). That was my understanding of the use-case.

[kozlovic]

That's why maybe don't do the changes until it is agreed with the rest of the team. I did not mean to have you make all the changes if you have to put it back because this is how the team wants the feature to work.

If as a team it is agreed that it should override, then simply add to the doc that this function ignores the "allow reconnect" option (currently it says that it respects them all).

[levb]

(will not merge until verified)

[levb]

See the comment from the team feedback: ignore allowReconnect altogether, just close the socket and let the client do what it does.

[kozlovic]

In my view, we should return an error in natsConnection_Reconnect() if the user explicitly disallowed reconnect with the natsOptions_SetAllowReconnect(opts, false). It does not make sense otherwise.

[kozlovic]

Well, actually not all reconnect options if we consider that "allow reconnect" is a reconnect option ;-). So I would argue that either the Go client should be changed, or the spec modified to specifically cover cases of libraries have an option to disable reconnection (maybe not all do that).

[kozlovic]

LGTM, but ping me for another review if the team decides to override the allow reconnect option.

[levb]

https://synadiacommunications.slack.com/archives/C05CGNTKNF2/p1715003032479599?thread_ts=1715000908.579469&cid=C05CGNTKNF2

So, so special treatment, do not return an error from Reconnect if allowReconnect==false, just kill the socket and let the client do what it does (error out).

[kozlovic]

LGTM. (Unrelated: the kv cross domains test is taking 20+ secs now and since you repeat the tests 3 times for some runs, it starts to add up. May want to see if that can be optimized in the test or if this is just the result from the server updates)

[levb]

@kozlovic yes, I knew it when I committed. I was hoping to soon get the word from @jnmoyne that the timeout can be shortened, but yes, let me at least make all the "long" tests run once. TBH I am yet to see any new flakes appear from the x3 change, so maybe it should be changed back to x1 for all, like it was before.