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 #4569: accounting for 0 timeouts that are not supported by jdk #4570

Merged
merged 1 commit into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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