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

Proposed update to enable reconnect on first attempt #223

Merged
merged 1 commit into from Dec 29, 2021

Conversation

pozsgaic
Copy link

We have had issues connecting on the first attempt, similar to issue brought up in the past:
#99

We modified the nats.rs to enable us to enter the retry loop on the first connection attempt. Our application behavior is such that we do not require the NatsServer be up and running when we start. This change allows for that behavior.

@derekcollison
Copy link
Member

We have some other client implementations that allow this on an opt-in behavior IIRC. We should make this consistent with them IMO.

@kozlovic
Copy link
Member

We do have it in Go, C, Node, etc.. indeed. In Go, the option is called "RetryOnFailedConnect".

@pozsgaic
Copy link
Author

OK thanks. Will review and follow up with an update.

@pozsgaic
Copy link
Author

Hi,
I have made updates to the code to use recommended naming. Please review.
Thanks

@derekcollison
Copy link
Member

Thanks, will take a look early next week!

@abalmos
Copy link

abalmos commented Oct 16, 2021

Just showing some additional interest for this to land

@caspervonb caspervonb requested review from caspervonb and removed request for spacejam November 2, 2021 09:42
@Jarema Jarema self-requested a review November 2, 2021 10:27
src/asynk.rs Outdated Show resolved Hide resolved
src/asynk.rs Outdated Show resolved Hide resolved
src/options.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
@Jarema
Copy link
Member

Jarema commented Nov 2, 2021

@pozsgaic Requested some minor changes in the PR. Thanks for your patience.

@pozsgaic
Copy link
Author

pozsgaic commented Nov 2, 2021

Thanks for reviewing the changes. I will address in the next few days.

@MattesWhite
Copy link
Contributor

We tested this PR as we desperately want this feature to reduce dependency on startup order of components in our system. Sadly, we observed that the retry_on_failed_connect() is a blocking operation and won't allow to proceed without a first connect, effectively blocking the rest of the startup code.
This might be intentional but I wanted to request to have an option to just progress while the Connection tries to establish a connection with a server. Just like when the Connection looses connection after the first successful connection (this sounds so weird). In addition, something like a fn last_communication(&self) -> Result<chrono::DateTime> would be nice to check when there was the last time client and server communicated if ever.

@Jarema
Copy link
Member

Jarema commented Dec 7, 2021

@MattesWhite Thanks for your feedback and testing. Much appreciated!

Changes can be adapted to what you said, so they work in similar fashion to Go reference client:

  • If RetryOnFailedConnect is not set, connection will be established when calling connect with either connection or error.
  • if RetryOnFailedConnect is set, client will still try to establish connection when connect is called but in case of failure, it will return Connection in reconnecting state and start connection process in separate thread.

To monitor state of the client, you can utilise optional callbacks (disconnect_callback, reconnect_callback, error_callback). I would not rely on any last_communication mechanism, as it's client responsibility to maintain it and inform user properly. We don't want force people to understand implementation details of protocol and guess when connection can be considered down.

Additional feedback is of course welcomed.

@Jarema
Copy link
Member

Jarema commented Dec 8, 2021

@pozsgaic To not duplicate work: I have working code for requirements described in my last comment.

Will add them after cleanup and testing.

EDIT: Thanks for applying review notes!

src/options.rs Show resolved Hide resolved
src/options.rs Outdated Show resolved Hide resolved
@Jarema
Copy link
Member

Jarema commented Dec 10, 2021

@MattesWhite To streamline the process we will merge this PR and follow it up with one having behaviour you described, as it needs some testing around informative errors coming from publish/subscribe and Jetstream if the server did not yet connect and user code started using those.

Until that happens (is on my list now so should be there really soon) you can wrap the connection into separate thread to achieve what you need if it's blocking your development.

@pozsgaic LGTM after pipeline is green. After pipeline is green, please squash the commits.

@Jarema
Copy link
Member

Jarema commented Dec 27, 2021

@pozsgaic Please squash the commits and we can merge :).

Signed-off-by: pozsgaic <chris.pozsgai@progress.com>
@pozsgaic
Copy link
Author

Sorry for the delay. Updated as requested. Thanks again!

@Jarema Jarema merged commit 3af59b5 into nats-io:main Dec 29, 2021
@Jarema
Copy link
Member

Jarema commented Dec 29, 2021

That you for your contribution! Merged.

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.

None yet

7 participants