Fix retries=0 bug in PortUtils.ConnectionCheckSSH #752
Conversation
pjdarton
commented
Oct 1, 2019
•
pjdarton
commented
- Issue Retries in ConnectionCheckSSH #751 - Math.min should've been Math.max
- Removed unused imports from unit test
- Fixed failure-case unit test for ConnectionCheckSSH
* Issue#751 - Math.min should've been Math.max * Removed unused imports from unit test * Added a failure-case unit test for ConnectionCheckSSH
| @@ -157,7 +157,7 @@ public ConnectionCheckSSH withSSHTimeout(int time, TimeUnit units) { | |||
| public boolean execute() throws InterruptedException { | |||
| checkState(parent.execute(), "Port %s is not opened to connect to", parent.port); | |||
|
|
|||
| final int retries = Math.min(0, parent.retries); | |||
| final int retries = Math.max(0, parent.retries); | |||
| @@ -172,16 +172,16 @@ public boolean execute() throws InterruptedException { | |||
| } | |||
|
|
|||
| private boolean executeOnce(final int thisTryNumber, final int totalTriesIntended) { | |||
| final Connection connection = new Connection(parent.host, parent.port); | |||
| final Connection sshConnection = new Connection(parent.host, parent.port); | |||
There was a problem hiding this comment.
I renamed the variable to make it clear that this was a SSH connection not merely a TCP connection that was being made.
| @@ -7,18 +7,15 @@ | |||
|
|
|||
| import java.io.IOException; | |||
| import java.net.ServerSocket; | |||
| import java.util.Date; | |||
There was a problem hiding this comment.
Removed some unused imports
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import static java.lang.System.currentTimeMillis; | ||
| import static java.util.concurrent.TimeUnit.MILLISECONDS; | ||
| import static java.util.concurrent.TimeUnit.SECONDS; | ||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.both; | ||
| import static org.hamcrest.Matchers.equalTo; |
There was a problem hiding this comment.
Replaced use of is with equalTo as "is" is deprecated.
| @@ -79,19 +76,24 @@ public void shouldThrowIllegalStateExOnNotAvailPort() throws Exception { | |||
| @Test | |||
| public void shouldWaitIfPortAvailableButNotSshUntilTimeout() throws Exception { | |||
There was a problem hiding this comment.
This was the buggy unit test that allowed the bug to go undetected.
|
@php50350 @biozshock This is the proposed fix for issue #751. Once the fix is confirmed I'll merge the changes. |
|
@pjdarton the URL does not work for me: same with http |
|
@biozshock That's weird. Maybe there's some internet firewall preventing you accessing that server? That server should be open to everyone on the internet... Anyway, I've attached (a zip containing) the .hpi file here, docker-plugin-1.1.9-rc895.e262f2331080.hpi.zip. Hopefully you'll be able to download it from here. |
|
Seems to be working correctly here. Thanks for quick fix @pjdarton |
|
OK, thanks for confirming the fix. |
|
We ran into the same issue (#751) with 1.1.8 and the 1.1.9 RC as provided above fixed it for us. Thank you. |
|
Code changes went into released of docker-plugin version 1.1.9 |
