added read timeout to http wait strategy #2058
Conversation
This commit adds the possibility to add a read timeout to the HTTP call for the HTTP wait strategy. This is necessary, because with the default Docker settings for Mac can lead to a blocking call to `HttpURLConnection.connect()`. The follow up issue is, that the container will never get healthy and due to automated integration tests the tests are not able to run. The main cause of the problem is the lazy connection establishment during setting up the container. So the time between starting the container and starting the wait strategy checks. The read timeout lets the HTTP call fail, so the `retryUntilSuccess()` is able to send another HTTP call in order to reestablish the connection and not block the whole thread.
| * @param timeout the timeout in millis | ||
| * @return this | ||
| */ | ||
| public HttpWaitStrategy withReadTimeout(int timeout) { |
There was a problem hiding this comment.
Could you please make it Duration?
There was a problem hiding this comment.
Done, the only problem is that you are possibly passing Duration.ofNanos(), but setReadTimeout() is not able to handle nanos, but millis.
There was a problem hiding this comment.
I would add a guard that checks that after calling toMillis() the amount is positive
There was a problem hiding this comment.
@dweidenfeld nice one! Just one small thing and we're good to go! 👍
| @@ -143,6 +157,7 @@ protected void waitUntilReady() { | |||
| getRateLimiter().doWhenReady(() -> { | |||
| try { | |||
| final HttpURLConnection connection = (HttpURLConnection) new URL(uri).openConnection(); | |||
| connection.setReadTimeout(Math.toIntExact(readTimeout.get(ChronoUnit.MILLIS))); | |||
There was a problem hiding this comment.
IIRC it should be readTimeout.toMillis(), otherwise you're only getting the milliseconds part of it
There was a problem hiding this comment.
Nice, but I have to adjust this tomorrow ;)
I'll leave a comment when I am ready.
There was a problem hiding this comment.
We plan to release tonight, I can do the review leftovers for you if you wish :)
There was a problem hiding this comment.
Okay I updated it just now. Please check it and if you need further adjustments feel free to do it yourself if you want ;)
bsideup
changed the title
|
@dweidenfeld merged, thank you! 👍 |
This commit adds the possibility to add a read timeout to the HTTP call
for the HTTP wait strategy.
This is necessary, because with the default Docker settings for Mac
can lead to a blocking call to
HttpURLConnection.connect(). The followup issue is, that the container will never get healthy and due to
automated integration tests the tests are not able to run.
The main cause of the problem is the lazy connection establishment
during setting up the container. So the time between starting the
container and starting the wait strategy checks.
The read timeout lets the HTTP call fail, so the
retryUntilSuccess()is able to send another HTTP call in order to reestablish the connection
and not block the whole thread.