Skip to content

Conversation

notoriaga
Copy link
Contributor

No description provided.

@notoriaga notoriaga changed the title add reconnection logic add reconnection logic [CPP-371] Oct 22, 2021
@notoriaga notoriaga requested a review from a team October 22, 2021 22:55
Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

Changes look good but we have quite a lot of thread::spawn calls now (which may be unavoidable given our architecture)-- can we create a diagram with draw.io that details how these threads interact? We should detail if threads are meant to be ephemeral or if they're long lived too.

.log_errors(log::Level::Debug)
.with_rover_time()
impl Messages {
const TIMEOUT: Duration = Duration::from_secs(30);
Copy link
Contributor

@silverjam silverjam Oct 23, 2021

Choose a reason for hiding this comment

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

I'm not seeing this timeout trigger, at least doesn't seem to work when tearing down the VPN:

console-reconnect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm strange, for me when I disconnect the vpn it triggers an error from the underlying tcp connection saying Resource temporarily unavailable (os error 11). Similar thing happens if I just disable wifi, haven't actually figured out a way to simulate no messages for 30 seconds without the io resource just returning an error itself

Copy link
Contributor

@silverjam silverjam Oct 24, 2021

Choose a reason for hiding this comment

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

Should be able to chain two socat instances together, one connected to the Piksi, connect another socat to the Piksi socat, console connects to this one, then kill the one connected to the Piksi.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any luck on reproducing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I just got another io error, BrokenPipe. I'm trying to shorten the timeout and then throttle my internet but that doesn't appear to work either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that does trigger the timeout for me, then after un-throttling it reconnects

@notoriaga
Copy link
Contributor Author

@silverjam Added a diagram of the backend threads. Not really sure how one typically draws something like this but for what I have each bar represents a thread. The left -> right layout represents parent/child relationships between threads.

@john-michaelburke does this seem accurate to you?
backend_threads_diagram

@silverjam
Copy link
Contributor

@silverjam Added a diagram of the backend threads. Not really sure how one typically draws something like this but for what I have each bar represents a thread. The left -> right layout represents parent/child relationships between threads.

Looks great, thanks

@notoriaga notoriaga merged commit f588159 into main Oct 25, 2021
@notoriaga notoriaga deleted the steve/reconnect branch October 25, 2021 21:01
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