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

Socketnetwork timeout and polling #411

Merged
merged 2 commits into from Sep 28, 2023

Conversation

JeppeOvervad
Copy link

No description provided.

@@ -137,7 +137,7 @@ private Map<Integer, Socket> connectClient(final NetworkConfiguration conf)
} catch (ConnectException e) {
// A ConnectionException is expected if the opposing side is not listening for our
// connection attempt yet. We ignore this and try again.
Thread.sleep(1L << ++attempts);
Thread.sleep(Math.min(1 << attempts++, 5_000));
Copy link
Author

Choose a reason for hiding this comment

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

The choice of an upper bound is based on the assumption that we always want to attempt to connect frequently - and 5 seconds was picked based on our own observations.

Alternatively this upper bound could be made configurable, or the entire calculation could be configurable - e.g. some Function<Integer, Long> pollInterval with the default being attempts -> 1 << attemps

Copy link
Member

Choose a reason for hiding this comment

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

It should probably just keep waiting the same amount, scale linearly, or max out after a period. We probably never want to wait 24 days (2^31 ms) between retries. But the existing implementation is a bit weird. 5 seconds is probably a good amount.

@JeppeOvervad
Copy link
Author

I assume the build failure is due to missing permissions on the job

https://github.com/aicis/fresco/blob/master/.github/workflows/test-report.yml ?

(Similar to 80c3c9b ? )

@quackzar
Copy link
Member

quackzar commented Sep 28, 2023

Yea, GitHub actions have a tendency to break with forks. Anyway the tests seem to run fine locally and the new constructor seems like a good candidate, so code coverage should not be a reason to block this.

@quackzar quackzar merged commit 660ffce into aicis:master Sep 28, 2023
1 check failed
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