fix #4911: consolidating several timeouts and refining requestTimeout

Author: shawkins

CHANGELOG.md

@@ -11,6 +11,9 @@
 #### New Features
 
 #### _**Note**_: Breaking changes
+* Fix #4911: Config/RequestConfig.scaleTimeout has been deprectated along with Scalable.scale(count, wait) and DeployableScalableResource.deployLatest(wait). withTimeout may be called before the operation to control the timeout.
+* Fix #4911: Config/RequestConfig.websocketTimeout has been removed. Config/RequestConfig.requestTimeout will be used for websocket connection timeouts.
+* Fix #4911: HttpClient api/building changes - writeTimeout has been removed, readTimeout has moved to the HttpRequest
 
 ### 6.6.2 (2023-05-15)
 
@@ -20,7 +23,7 @@ Fix #5121: RequestConfig is propagated to derived HttpClient instances
 ### 6.6.1 (2023-05-11)
 
 #### Bugs
-* Fix #5095: moving the enforcement to requestTimeout
+* Fix #5095: moving the enforcement of requestTimeout
 * Fix #5100: lessened the level of the non-conflicting httpclient implementation warning
 * Fix #5102: wait on scale to 0 was not completing
 * Fix #5112: Expose put method with InputStream argument in HttpRequest class

httpclient-jdk/src/main/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientImpl.java

@@ -60,7 +60,6 @@
 /**
  * TODO:
  * - Mapping to a Reader is always UTF-8
- * - determine if write timeout should be implemented
  */
 public class JdkHttpClientImpl extends StandardHttpClient<JdkHttpClientImpl, JdkHttpClientFactory, JdkHttpClientBuilderImpl> {
 
@@ -259,7 +258,7 @@ public CompletableFuture<HttpResponse<AsyncBody>> consumeBytesDirect(StandardHtt
   java.net.http.HttpRequest.Builder requestBuilder(StandardHttpRequest request) {
     java.net.http.HttpRequest.Builder requestBuilder = java.net.http.HttpRequest.newBuilder();
 
-    Duration readTimeout = this.builder.getReadTimeout();
+    Duration readTimeout = request.getReadTimeout();
     if (readTimeout != null && !java.time.Duration.ZERO.equals(readTimeout)) {
       requestBuilder.timeout(readTimeout);
     }
@@ -306,15 +305,13 @@ public long contentLength() {
   @Override
   public CompletableFuture<WebSocketResponse> buildWebSocketDirect(
       StandardWebSocketBuilder standardWebSocketBuilder, Listener listener) {
-    final StandardHttpRequest request = standardWebSocketBuilder.asHttpRequest();
+    StandardHttpRequest request = standardWebSocketBuilder.asHttpRequest();
     java.net.http.WebSocket.Builder newBuilder = this.getHttpClient().newWebSocketBuilder();
     request.headers().forEach((k, v) -> v.forEach(s -> newBuilder.header(k, s)));
     if (standardWebSocketBuilder.getSubprotocol() != null) {
       newBuilder.subprotocols(standardWebSocketBuilder.getSubprotocol());
     }
-    // the Watch logic sets a websocketTimeout as the readTimeout
-    // TODO: this should probably be made clearer in the docs
-    Duration readTimeout = this.builder.getReadTimeout();
+    Duration readTimeout = request.getReadTimeout();
     if (readTimeout != null && !java.time.Duration.ZERO.equals(readTimeout)) {
       newBuilder.connectTimeout(readTimeout);
     }

httpclient-jdk/src/test/java/io/fabric8/kubernetes/client/jdkhttp/JdkHttpClientBuilderTest.java

@@ -30,10 +30,7 @@ void testZeroTimeouts() {
     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();) {
+    try (HttpClient client = builder.connectTimeout(0, TimeUnit.MILLISECONDS).build();) {
       assertNotNull(client.newHttpRequestBuilder().uri("http://localhost").build());
     }
   }

httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyHttpClient.java

@@ -18,7 +18,6 @@
 import io.fabric8.kubernetes.client.KubernetesClientException;
 import io.fabric8.kubernetes.client.http.AsyncBody;
 import io.fabric8.kubernetes.client.http.AsyncBody.Consumer;
-import io.fabric8.kubernetes.client.http.HttpRequest;
 import io.fabric8.kubernetes.client.http.HttpResponse;
 import io.fabric8.kubernetes.client.http.StandardHttpClient;
 import io.fabric8.kubernetes.client.http.StandardHttpClientBuilder;
@@ -100,7 +99,9 @@ private Request newRequest(StandardHttpRequest originalRequest) {
     final var request = requestBuilder.build();
 
     var jettyRequest = jetty.newRequest(request.uri()).method(request.method());
-    jettyRequest.timeout(builder.getReadTimeout().toMillis() + builder.getWriteTimeout().toMillis(), TimeUnit.MILLISECONDS);
+    if (originalRequest.getReadTimeout() != null) {
+      jettyRequest.timeout(originalRequest.getReadTimeout().toMillis(), TimeUnit.MILLISECONDS);
+    }
     jettyRequest.headers(m -> request.headers().forEach((k, l) -> l.forEach(v -> m.add(k, v))));
 
     final var contentType = Optional.ofNullable(request.getContentType());
@@ -136,14 +137,14 @@ public CompletableFuture<WebSocketResponse> buildWebSocketDirect(StandardWebSock
       Listener listener) {
     try {
       jettyWs.start();
-      final HttpRequest request = standardWebSocketBuilder.asHttpRequest();
+      StandardHttpRequest request = standardWebSocketBuilder.asHttpRequest();
       final ClientUpgradeRequest cur = new ClientUpgradeRequest();
       if (Utils.isNotNullOrEmpty(standardWebSocketBuilder.getSubprotocol())) {
         cur.setSubProtocols(standardWebSocketBuilder.getSubprotocol());
       }
       cur.setHeaders(request.headers());
-      if (builder.getReadTimeout() != null) {
-        cur.setTimeout(builder.getReadTimeout().toMillis(), TimeUnit.MILLISECONDS);
+      if (request.getReadTimeout() != null) {
+        cur.setTimeout(request.getReadTimeout().toMillis(), TimeUnit.MILLISECONDS);
       }
       // Extra-future required because we can't Map the UpgradeException to a WebSocketHandshakeException easily
       final CompletableFuture<WebSocketResponse> future = new CompletableFuture<>();

httpclient-jetty/src/main/java/io/fabric8/kubernetes/client/jetty/JettyHttpClientBuilder.java

@@ -111,16 +111,6 @@ protected JettyHttpClientBuilder newInstance(JettyHttpClientFactory clientFactor
     return new JettyHttpClientBuilder(clientFactory);
   }
 
-  @Override
-  public Duration getReadTimeout() {
-    return Optional.ofNullable(readTimeout).orElse(Duration.ZERO);
-  }
-
-  @Override
-  public Duration getWriteTimeout() {
-    return Optional.ofNullable(writeTimeout).orElse(Duration.ZERO);
-  }
-
   @Override
   public Duration getConnectTimeout() {
     return Optional.ofNullable(connectTimeout).orElse(Duration.ZERO);

httpclient-jetty/src/test/java/io/fabric8/kubernetes/client/jetty/JettyHttpClientTest.java

@@ -68,14 +68,12 @@ void newBuilderInstantiatesJettyHttpClientBuilderWithSameSettings() throws Excep
     final var originalBuilder = new JettyHttpClientBuilder(null);
     originalBuilder
         .connectTimeout(1337, TimeUnit.SECONDS)
-        .readTimeout(1337, TimeUnit.SECONDS)
         .tlsVersions(TlsVersion.SSL_3_0)
         .followAllRedirects();
     try (var firstClient = new JettyHttpClient(
         originalBuilder, httpClient, webSocketClient)) {
       // When
-      final var result = firstClient.newBuilder()
-          .readTimeout(313373, TimeUnit.SECONDS);
+      final var result = firstClient.newBuilder();
       // Then
       assertThat(result)
           .isNotNull()
@@ -90,11 +88,11 @@ void newBuilderInstantiatesJettyHttpClientBuilderWithSameSettings() throws Excep
             .isEqualTo(method.invoke(originalBuilder))
             .isEqualTo(entry.getValue());
       }
-      var readTimeout = StandardHttpClientBuilder.class.getDeclaredField("readTimeout");
-      readTimeout.setAccessible(true);
-      assertThat(readTimeout.get(result)).isEqualTo(Duration.ofSeconds(313373));
-      assertThat(readTimeout.get(originalBuilder)).isEqualTo(Duration.ofSeconds(1337));
-      readTimeout.setAccessible(false);
+      var connectTimeout = StandardHttpClientBuilder.class.getDeclaredField("connectTimeout");
+      connectTimeout.setAccessible(true);
+      assertThat(connectTimeout.get(result)).isEqualTo(Duration.ofSeconds(1337));
+      assertThat(connectTimeout.get(originalBuilder)).isEqualTo(Duration.ofSeconds(1337));
+      connectTimeout.setAccessible(false);
     }
   }
 

httpclient-okhttp/src/main/java/io/fabric8/kubernetes/client/okhttp/OkHttpClientBuilderImpl.java

@@ -91,18 +91,9 @@ public OkHttpClientImpl initialBuild(okhttp3.OkHttpClient.Builder builder) {
   }
 
   private OkHttpClientImpl derivedBuild(okhttp3.OkHttpClient.Builder builder) {
-    if (readTimeout != null) {
-      builder.readTimeout(this.readTimeout);
-    }
-    if (writeTimeout != null) {
-      builder.writeTimeout(this.writeTimeout);
-    }
     if (authenticatorNone) {
       builder.authenticator(Authenticator.NONE);
     }
-    if (forStreaming) {
-      builder.cache(null);
-    }
     OkHttpClient client = builder.build();
 
     return new OkHttpClientImpl(client, this);

httpclient-okhttp/src/main/java/io/fabric8/kubernetes/client/okhttp/OkHttpClientImpl.java

@@ -291,10 +291,24 @@ public void close() {
     }
   }
 
-  private CompletableFuture<HttpResponse<AsyncBody>> sendAsync(HttpRequest request,
+  private CompletableFuture<HttpResponse<AsyncBody>> sendAsync(StandardHttpRequest request,
       Function<BufferedSource, AsyncBody> handler) {
     CompletableFuture<HttpResponse<AsyncBody>> future = new CompletableFuture<>();
-    Call call = httpClient.newCall(requestBuilder((StandardHttpRequest) request).build());
+
+    okhttp3.OkHttpClient.Builder clientBuilder = null;
+    if (request.getReadTimeout() != null) {
+      clientBuilder = httpClient.newBuilder();
+      clientBuilder.readTimeout(request.getReadTimeout());
+    }
+    if (request.isForStreaming()) {
+      if (clientBuilder == null) {
+        clientBuilder = httpClient.newBuilder();
+      }
+      clientBuilder.cache(null);
+    }
+
+    Call call = Optional.ofNullable(clientBuilder).map(okhttp3.OkHttpClient.Builder::build).orElse(httpClient)
+        .newCall(requestBuilder(request).build());
     try {
       call.enqueue(new Callback() {
 

httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpClient.java

@@ -63,16 +63,16 @@ public CompletableFuture<WebSocketResponse> buildWebSocketDirect(StandardWebSock
       WebSocket.Listener listener) {
     WebSocketConnectOptions options = new WebSocketConnectOptions();
 
-    if (builder.getReadTimeout() != null) {
-      options.setTimeout(builder.getReadTimeout().toMillis());
-    }
-
     if (standardWebSocketBuilder.getSubprotocol() != null) {
       options.setSubProtocols(Collections.singletonList(standardWebSocketBuilder.getSubprotocol()));
     }
 
     final StandardHttpRequest request = standardWebSocketBuilder.asHttpRequest();
 
+    if (request.getReadTimeout() != null) {
+      options.setTimeout(request.getReadTimeout().toMillis());
+    }
+
     request.headers().entrySet().stream()
         .forEach(e -> e.getValue().stream().forEach(v -> options.addHeader(e.getKey(), v)));
     options.setAbsoluteURI(request.uri().toString());
@@ -107,6 +107,10 @@ public CompletableFuture<HttpResponse<AsyncBody>> consumeBytesDirect(StandardHtt
     options.setAbsoluteURI(request.uri().toString());
     options.setMethod(HttpMethod.valueOf(request.method()));
 
+    if (request.getReadTimeout() != null) {
+      options.setTimeout(request.getReadTimeout().toMillis());
+    }
+
     // Proxy authorization is handled manually since the proxyAuthorization value is the actual header
     if (proxyAuthorization != null) {
       options.putHeader(HttpHeaders.PROXY_AUTHORIZATION, proxyAuthorization);

httpclient-vertx/src/main/java/io/fabric8/kubernetes/client/vertx/VertxHttpClientBuilder.java

@@ -61,10 +61,6 @@ public VertxHttpClient<F> build() {
       options.setConnectTimeout((int) this.connectTimeout.toMillis());
     }
 
-    if (this.writeTimeout != null) {
-      options.setWriteIdleTimeout((int) this.writeTimeout.getSeconds());
-    }
-
     if (this.followRedirects) {
       options.setFollowRedirects(followRedirects);
     }