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

fix: await during DockerClientProviderStrategy test method #9412

Conversation

KyleAure
Copy link
Contributor

Current behavior

The current test method performs the following socket test:

try (Socket socket = socketProvider.call()) {
Duration timeout = Duration.ofMillis(200);
Awaitility
.await()
.atMost(TestcontainersConfiguration.getInstance().getClientPingTimeout(), TimeUnit.SECONDS)
.pollInterval(timeout)
.pollDelay(Duration.ofSeconds(0)) // start checking immediately
.ignoreExceptionsInstanceOf(SocketTimeoutException.class)
.untilAsserted(() -> socket.connect(socketAddress, (int) timeout.toMillis()));
return true;
} catch (Exception e) {
log.warn("DOCKER_HOST {} is not listening", dockerHost);
return false;
}
}

Using the config client.ping.timeout

client.ping.timeout = 5 Specifies for how long Testcontainers will try to connect to the Docker client to obtain valid info about the client before giving up and trying next strategy, if applicable (in seconds).

Consider the following scenario where I set client.ping.timeout=60 because my docker host and local machine are in different locales and I expect a network lag.

Duration timeout = Duration.ofMillis(200);
Awaitility
    .await()
    .atMost(TestcontainersConfiguration.getInstance().getClientPingTimeout(), TimeUnit.SECONDS) // do not wait more than 60 seconds
    .pollInterval(timeout) // check status every 200ms
    .pollDelay(Duration.ofSeconds(0)) // start checking immediately
    .ignoreExceptionsInstanceOf(SocketTimeoutException.class) // Ignore the expected timeout
    .untilAsserted(() -> socket.connect(socketAddress, (int) timeout.toMillis())); // Try to connect and timeout after 200ms

Currently, the client.ping.timeout configuration is essentially ignored because the socket will always timeout before atMost is ever reached.

Proposed Behavior

Instead, we should not provide a timeout to socket.connect.
If we take the same example as previously mentioned:

Awaitility
    .await()
    .atMost(TestcontainersConfiguration.getInstance().getClientPingTimeout(), TimeUnit.SECONDS)  // do not wait more than 60 seconds
    .pollInterval(Duration.ofMillis(200)) // check state every 200ms
    .pollDelay(Duration.ofSeconds(0)) // start checking immediately
    .untilAsserted(() -> socket.connect(socketAddress)); // Try to connect with an unlimited timeout

If the client.ping.timeout is reached, then Awaitility will interrupt the Runnable and still throw a ConditionTimeoutException.

@KyleAure KyleAure requested a review from a team as a code owner October 16, 2024 21:05
@eddumelendez eddumelendez added this to the next milestone Oct 21, 2024
@eddumelendez eddumelendez modified the milestone: next Oct 22, 2024
@eddumelendez eddumelendez merged commit 119627f into testcontainers:main Oct 22, 2024
104 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @KyleAure !

@KyleAure KyleAure deleted the bug-DockerClientProviderStrategy-awatility branch October 23, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants