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

Fix jetstream reconnect #610

Merged
merged 4 commits into from
Aug 26, 2022
Merged

Fix jetstream reconnect #610

merged 4 commits into from
Aug 26, 2022

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented Aug 15, 2022

  1. Added sending new request when disconnect followed by connect state is detected
  2. Added checking for timeout of pull request
  3. improved nats-server crate to allow restarts

@Jarema Jarema requested a review from caspervonb August 15, 2022 11:28
@Jarema Jarema changed the title Jarema/fix jetstream reconnect Fix jetstream reconnect Aug 15, 2022
@Jarema Jarema force-pushed the jarema/fix-jetstream-reconnect branch from e6a7883 to 3ae2a7e Compare August 18, 2022 15:41
@Jarema Jarema marked this pull request as ready for review August 23, 2022 13:05
@Jarema Jarema force-pushed the jarema/fix-jetstream-reconnect branch 2 times, most recently from 56e9836 to 9c65c78 Compare August 24, 2022 20:24
context.client.state.changed().await.ok();
let state = context.client.state.borrow().to_owned();

let timeout_threshold = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Collapse this into map_or?

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. fixed.

@@ -392,6 +397,7 @@ pub struct Stream<'a> {
subject: String,
batch_config: BatchConfig,
request: Option<BoxFuture<'a, Result<(), Error>>>,
since_last_request: Arc<Mutex<Instant>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't have to point this out but, obviously preferable to not have a mutex here.
But for a fix, we can allow it with the intent to iterate soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we could go with pending as atomic shared between task and iterator fn, with Instant hold on task only and having task only sending requests, but that also needs returning error from it via channel to iterator. I would prefer revisit it soon and now focus on more important work.

@@ -1280,6 +1281,73 @@ mod jetstream {
assert_eq!(info.num_ack_pending, 8);
}

#[tokio::test]
#[cfg_attr(target_os = "windows", ignore)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I take it that Windows reconnect remain flaky?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, i actually changed the reconnections, allowing reset of server, but I would limit windows tests where feasible. Those windows gha runners are so slow they have weird impact on nats-server and connection.

@@ -24,6 +24,13 @@ use rand::Rng;
use regex::Regex;

pub struct Server {
inner: Inner,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the indirection?

Copy link
Member Author

@Jarema Jarema Aug 25, 2022

Choose a reason for hiding this comment

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

Because if you would assing new server directly to *self, drop() is called, which wipes the data etc. i want to keep wiping data on drop.
Other solution is to return big tuple from run_server internal fn, but thats ugly.

inner looked like most convenient and clean way to approach this.

@Jarema Jarema requested a review from caspervonb August 26, 2022 12:33
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

Think we'll revisit the internals of this in the future but lgtm 👍

@Jarema Jarema force-pushed the jarema/fix-jetstream-reconnect branch from 1af29ce to 3af83a1 Compare August 26, 2022 14:33
@Jarema Jarema merged commit 4564588 into main Aug 26, 2022
@Jarema Jarema deleted the jarema/fix-jetstream-reconnect branch August 26, 2022 15:43
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

2 participants