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

Have connectBlocking clean up after a timeout #1399

Merged
merged 2 commits into from Feb 24, 2024

Conversation

ysi-camerona
Copy link
Contributor

@ysi-camerona ysi-camerona commented Feb 21, 2024

Related Issue

Fixes #1397

Motivation and Context

Previously, connectBlocking(long, TimeUnit) would return a boolean indicating whether the connection was set up in the given window. If false was returned, we would continue to attempt connecting in the background. This PR updates connectBlocking(long, TimeUnit) to perform cleanup on timeout.

How Has This Been Tested?

  • Unit tests have been run
  • See "To Reproduce" steps on connectTimeout not fully respected #1397 - I used the firewall rules listed, and then updated the sample app linked to run the default client in a in a loop (500 iterations) to verify there were no thread or memory leaks

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change) - connectBlocking(long, TimeUnit) behavior has changed

Checklist:

  • My code follows the code style of this project.
  • All existing tests passed.

Note: I spent a couple hours spiking a unit test around this change but couldn't come up with anything solid. I found it difficult to simulate the "drop packets mid-connection" firewall rule listed in the linked issue. If we think this is necessary to test, advice on testing would be appreciated.

…on is not yet opened. This prevents the write thread from hanging while attempting to write to the output stream. Reset the connection if we fail to connect during connectBlocking.
@PhilipRoman
Copy link
Collaborator

I think I know how to make a unit test for this, I will implement it in the next few days.

@PhilipRoman PhilipRoman merged commit aa84b39 into TooTallNate:master Feb 24, 2024
1 check passed
PhilipRoman added a commit that referenced this pull request Apr 27, 2024
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.

connectTimeout not fully respected
2 participants