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

Make PortRegistry avoid ephemeral ports #54

Merged
merged 2 commits into from
Dec 5, 2016
Merged

Conversation

bentmann
Copy link
Contributor

@bentmann bentmann commented Dec 4, 2016

http://bamboo.s/browse/OSS-GOODIESF19-1

Some of the sporadic IT failures we see are due to "reserved" ports being unavailable for their intended use because the OS reused the port in the meantime. So far I had been trying to identify and fix those other port usages which are however tedious to locate, hard to change and lastly fragile with regard to future code evolution. Today it finally dawned on me that there is a more general cure, addressing the root cause: Not using ephemeral ports to begin with. Manually scanning a port range for a free one isn't rocket science, sure, the OS might do it faster but with only ~10 seconds to scan 64k ports the manual search is hardly an issue.

…hereby avoid sporadic clashes of reserved ports with ephemeral ports reused by the operation system for other sockets
@@ -40,8 +41,6 @@
*/
public class PortRegistry
{
private static final int MAX_ATTEMPTS = 10;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One can't block larger ranges like [21000, 21099], find a free port at 21001, advance from there in increments of 1 and try to get out of the block with only 10 attempts. I argue that trying to find a sufficiently big number to limit the attempts is futile, what one really cares about is not having an infinite port search, something a timeout can (more directly) provide.

Copy link
Contributor

@mcculls mcculls left a comment

Choose a reason for hiding this comment

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

+1 good improvement


private final int reservationTimeout;

private int nextPort;

Choose a reason for hiding this comment

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

Could be a local variable in findFreePortManual

Copy link
Contributor Author

@bentmann bentmann Dec 5, 2016

Choose a reason for hiding this comment

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

No, that variable tracks the next port which is (likely) free to use for the next call to reserverPort()

Choose a reason for hiding this comment

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

Right, makes sense. Never mind.

@ataylor284
Copy link

+1

@bentmann bentmann merged commit 5140bb9 into master Dec 5, 2016
@bentmann bentmann deleted the smarter-port-registry branch December 5, 2016 17:28
@bentmann
Copy link
Contributor Author

bentmann commented Dec 5, 2016

The thing that I overlooked: When one JVM launches a child JVM, the state of the port registry is not shared across those two JVMs. In particular, the child JVM does not know which ports the parent JVM has already reserved. When letting the OS pick an ephemeral port, this issue is mostly avoided due to the OS basically maintaining a global "nextPort". But the manual port selection is prone to hand out ports in the child JVM that the parent JVM already reserved...

I'll craft a follow-up PR to try and resolve that

@bentmann
Copy link
Contributor Author

bentmann commented Dec 6, 2016

Said issue with child JVMs can be solved neatly already using the new constructor by configuring the child JVM to use a different port range than its parent. So all good.

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