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

refactor(client): explicit timeout during connect #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

milgner
Copy link
Member

@milgner milgner commented Nov 4, 2022

We observed a scenario where the client was stuck trying to connect. Most likely cause is that packets were DROPed from the network instead of being REJECTed. This adds an experimental explicit timeout to the TcpStream::connect call.

Copy link
Member

@robsliwi robsliwi left a comment

Choose a reason for hiding this comment

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

timeout

}
};
// Wait for connection state to be closed
let mut timer = interval(Duration::from_millis(10));
Copy link
Member

Choose a reason for hiding this comment

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

Wonder were this magic number comes from. Or is it part of the spec? 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably just tried & tested? I'm not that familiar with the code to really say. Just looking at it now, it's even possible that the whole loop here could be removed because all the work happens inside the spawn_looping_tasks that build upon the read- and write-halfs of the connection and this task doesn't really need to do anything anymore.

Comment on lines +417 to +418
debug! {"Sending HELLO"}
;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
debug! {"Sending HELLO"}
;
debug! {"Sending HELLO"};

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.

3 participants