Skip to content

Commit

Permalink
fix #4569: accounting for 0 timeouts that are not supported by jdk
Browse files Browse the repository at this point in the history
  • Loading branch information
shawkins authored and manusa committed Nov 15, 2022
1 parent a711b90 commit 4ac5137
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* Fix #4535: The shell command string will now have single quotes sanitized
* Fix #4543: (Java Generator) additionalProperties JsonAny setter method generated as setAdditionalProperty
* Fix #4547: preventing timing issues with leader election cancel
* Fix #4569: fixing jdk httpclient regression with 0 timeouts

#### Improvements
* Fix #4355: for exec, attach, upload, and copy operations the container id/name will be validated or chosen prior to the remote call. You may also use the kubectl.kubernetes.io/default-container annotation to specify the default container.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public HttpClient build() {
return new JdkHttpClientImpl(this, client.getHttpClient(), this.requestConfig);
}
java.net.http.HttpClient.Builder builder = clientFactory.createNewHttpClientBuilder();
if (connectTimeout != null) {
if (connectTimeout != null && !java.time.Duration.ZERO.equals(connectTimeout)) {
builder.connectTimeout(connectTimeout);
}
if (sslContext != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.net.http.WebSocketHandshakeException;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -404,8 +405,9 @@ public CompletableFuture<WebSocketResponse> internalBuildAsync(JdkWebSocketImpl.
}
// the Watch logic sets a websocketTimeout as the readTimeout
// TODO: this should probably be made clearer in the docs
if (this.builder.getReadTimeout() != null) {
newBuilder.connectTimeout(this.builder.getReadTimeout());
Duration readTimeout = this.builder.getReadTimeout();
if (readTimeout != null && !java.time.Duration.ZERO.equals(readTimeout)) {
newBuilder.connectTimeout(readTimeout);
}

AtomicLong queueSize = new AtomicLong();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public Builder setHeader(String k, String v) {
}

public Builder timeout(Duration duration) {
if (duration != null) {
if (duration != null && !Duration.ZERO.equals(duration)) {
builder.timeout(duration);
}
return this;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* Copyright (C) 2015 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.kubernetes.client.jdkhttp;

import io.fabric8.kubernetes.client.http.HttpClient;
import org.junit.jupiter.api.Test;

import java.util.concurrent.TimeUnit;

import static org.junit.jupiter.api.Assertions.assertNotNull;

class JdkHttpClientBuilderTest {

@Test
void testZeroTimeouts() {
JdkHttpClientFactory factory = new JdkHttpClientFactory();
JdkHttpClientBuilderImpl builder = factory.newBuilder();

// should build and be usable without an issue
try (HttpClient client = builder.readTimeout(0, TimeUnit.MILLISECONDS).connectTimeout(0, TimeUnit.MILLISECONDS)
.writeTimeout(0,
TimeUnit.MILLISECONDS)
.build();) {
assertNotNull(client.newHttpRequestBuilder().uri("http://localhost").build());
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,18 @@ void sendEmitsMessageToWebSocketServer() throws Exception {
.done()
.always();
final BlockingQueue<String> receivedText = new ArrayBlockingQueue<>(1);
final WebSocket ws = client.newWebSocketBuilder()
final WebSocket ws = client
// ensure that both a derived builder and a 0, or no, timeout works
// as that is a common logic path in the client
.newBuilder().readTimeout(0, TimeUnit.SECONDS).build().newWebSocketBuilder()
// TODO: JDK HttpClient implementation doesn't work with ws URIs
// - Currently we are using an HttpRequest.Builder which is then
// mapped to a WebSocket.Builder. We should probably user the WebSocket.Builder
// directly
//.uri(URI.create(String.format("ws://%s:%s/send-text", server.getHostName(), server.getPort())))
.uri(URI.create(server.url("send-text")))
.buildAsync(new WebSocket.Listener() {
@Override
public void onMessage(WebSocket webSocket, String text) {
assertTrue(receivedText.offer(text));
}
Expand Down

0 comments on commit 4ac5137

Please sign in to comment.