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.

Sign up for GitHub

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

Jump to bottom

"Use random ports" may bind to already used ports #138

Closed
f4lco opened this issue Jan 14, 2020 · 1 comment · Fixed by #139
Closed

"Use random ports" may bind to already used ports #138

f4lco opened this issue Jan 14, 2020 · 1 comment · Fixed by #139
Labels
kind/bug Something isn't working

Comments

@f4lco
Copy link
Collaborator

f4lco commented Jan 14, 2020

We use Gretty mainly for integration tests. We observed a test flakiness stemming from Gretty, which tried to bind already used ports when configuring the "use random ports" feature.

The analysis is as follows:

  1. Gretty chooses ports (for Tomcat/Jetty, status port, service port) upfront. It picks a random free port by binding to 0, and unbinds it immediately again.
  2. At a later stage, the previously identified port gets passed to server configurer, or the respective server sockets for internal communication ("service protocol").

In between choosing a free port and binding it there exists a sufficient amount of time for another process to chip in and bind the same port, causing the second step to fail for one of the processes.

I think a fix consists of the following two steps:

  • Ensuring that ports are not chosen upfront, but rather by the individual server connectors - they have to bind with '0', and then the actually bound port has to be retrieved via the localPort property.
  • The service protocol must be reworked, such that the ports of the two server sockets are bound as long as possible (ideally over the entire execution of Gretty from deployment to tear-down if applicable).

I have a patch ready and I am currently testing it. Are you interested in a PR?
@javabrett
Copy link
Member

Thanks @f4lco yes a PR would be welcome.

f4lco added a commit to f4lco/gretty that referenced this issue Feb 3, 2020
@f4lco
Delegate choosing random port to Jetty / Tomcat (gretty-gradle-plugin…
14abdba 
…#138)
f4lco added a commit to f4lco/gretty that referenced this issue Feb 3, 2020
@f4lco
Bind and use auxiliary ports (servicePort, statusPort) immediately to…
a8276a9 
… avoid race conditions (gretty-gradle-plugin#138)

API note: the server configuration "auxPortRange" is not supported any
more.
@javabrett javabrett added the kind/bug Something isn't working label Feb 8, 2020
@javabrett javabrett linked a pull request Feb 8, 2020 that will close this issue
Choose random free ports atomically #139
Merged
f4lco added a commit to f4lco/gretty that referenced this issue Apr 4, 2020
@f4lco
Remove malformed check for already used ports (fixes gretty-gradle-pl…
b9c9127 
…ugin#147)

Because gretty-gradle-plugin#138 removed adequate error handling, the check for
already used ports was broken and caused Gretty to fail in
case the 'gretty_ports.properties' file was present.

I think the check is now obsolete because gretty-gradle-plugin#138 removed the
ability to manually specify fixed service and status ports
for Gretty. As a result, Gretty's ports will never clash
from one Gretty invocation to the next.

If the check was restored, it would only check if the
previous Gretty invocation had already terminated.

If one of the *servlet container ports* clashed, I deem
the current behavior (BindException) sufficient.
f4lco added a commit to f4lco/gretty that referenced this issue Apr 4, 2020
@f4lco
Remove malformed check for already used ports (fixes gretty-gradle-pl…
3705abb 
…ugin#147)

Because gretty-gradle-plugin#138 removed adequate error handling, the check for
already used ports was broken and caused Gretty to fail in
case the 'gretty_ports.properties' file was present.

I think the check is now obsolete because gretty-gradle-plugin#138 removed the
ability to manually specify fixed service and status ports
for Gretty. As a result, Gretty's ports will never clash
from one Gretty invocation to the next.

If the check was restored, it would only check if the
previous Gretty invocation had already terminated.

If one of the *servlet container ports* clashed, I deem
the current behavior (BindException) sufficient.
This was referenced Apr 4, 2020
Merged
Closed
f4lco added a commit to f4lco/gretty that referenced this issue Apr 13, 2020
@f4lco
Remove malformed check for already used ports (fixes gretty-gradle-pl…
2219379 
…ugin#147)

Because gretty-gradle-plugin#138 removed adequate error handling, the check for
already used ports was broken and caused Gretty to fail in
case the 'gretty_ports.properties' file was present.

I think the check is now obsolete because gretty-gradle-plugin#138 removed the
ability to manually specify fixed service and status ports
for Gretty. As a result, Gretty's ports will never clash
from one Gretty invocation to the next.

If the check was restored, it would only check if the
previous Gretty invocation had already terminated.

If one of the *servlet container ports* clashed, I deem
the current behavior (BindException) sufficient.
f4lco added a commit to f4lco/gretty that referenced this issue May 3, 2020
@f4lco
Remove malformed check for already used ports (fixes gretty-gradle-pl…
74ad225 
…ugin#147)

Because gretty-gradle-plugin#138 removed adequate error handling, the check for
already used ports was broken and caused Gretty to fail in
case the 'gretty_ports.properties' file was present.

I think the check is now obsolete because gretty-gradle-plugin#138 removed the
ability to manually specify fixed service and status ports
for Gretty. As a result, Gretty's ports will never clash
from one Gretty invocation to the next.

If the check was restored, it would only check if the
previous Gretty invocation had already terminated.

If one of the *servlet container ports* clashed, I deem
the current behavior (BindException) sufficient.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Choose random free ports atomically f4lco/gretty
2 participants
@javabrett @f4lco