Skip to content

Commit

Permalink
fix fabric8io#4911: redefining readTimeout as just HttpRequest.timeout
Browse files Browse the repository at this point in the history
  • Loading branch information
shawkins committed May 16, 2023
1 parent f6d0b6b commit 924d788
Show file tree
Hide file tree
Showing 12 changed files with 42 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,9 @@ 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 = request.getReadTimeout();
if (readTimeout != null && !java.time.Duration.ZERO.equals(readTimeout)) {
requestBuilder.timeout(readTimeout);
Duration timeout = request.getTimeout();
if (timeout != null && !java.time.Duration.ZERO.equals(timeout)) {
requestBuilder.timeout(timeout);
}

request.headers().entrySet().stream()
Expand Down Expand Up @@ -311,9 +311,9 @@ public CompletableFuture<WebSocketResponse> buildWebSocketDirect(
if (standardWebSocketBuilder.getSubprotocol() != null) {
newBuilder.subprotocols(standardWebSocketBuilder.getSubprotocol());
}
Duration readTimeout = request.getReadTimeout();
if (readTimeout != null && !java.time.Duration.ZERO.equals(readTimeout)) {
newBuilder.connectTimeout(readTimeout);
Duration timeout = request.getTimeout();
if (timeout != null && !java.time.Duration.ZERO.equals(timeout)) {
newBuilder.connectTimeout(timeout);
}

AtomicLong queueSize = new AtomicLong();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ private Request newRequest(StandardHttpRequest originalRequest) {
final var request = requestBuilder.build();

var jettyRequest = jetty.newRequest(request.uri()).method(request.method());
if (originalRequest.getReadTimeout() != null) {
jettyRequest.timeout(originalRequest.getReadTimeout().toMillis(), TimeUnit.MILLISECONDS);
if (originalRequest.getTimeout() != null) {
jettyRequest.timeout(originalRequest.getTimeout().toMillis(), TimeUnit.MILLISECONDS);
}
jettyRequest.headers(m -> request.headers().forEach((k, l) -> l.forEach(v -> m.add(k, v))));

Expand Down Expand Up @@ -143,8 +143,8 @@ public CompletableFuture<WebSocketResponse> buildWebSocketDirect(StandardWebSock
cur.setSubProtocols(standardWebSocketBuilder.getSubprotocol());
}
cur.setHeaders(request.headers());
if (request.getReadTimeout() != null) {
cur.setTimeout(request.getReadTimeout().toMillis(), TimeUnit.MILLISECONDS);
if (request.getTimeout() != null) {
cur.setTimeout(request.getTimeout().toMillis(), TimeUnit.MILLISECONDS);
}
// Extra-future required because we can't Map the UpgradeException to a WebSocketHandshakeException easily
final CompletableFuture<WebSocketResponse> future = new CompletableFuture<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,9 @@ private CompletableFuture<HttpResponse<AsyncBody>> sendAsync(StandardHttpRequest
CompletableFuture<HttpResponse<AsyncBody>> future = new CompletableFuture<>();

okhttp3.OkHttpClient.Builder clientBuilder = null;
if (request.getReadTimeout() != null) {
if (request.getTimeout() != null) {
clientBuilder = httpClient.newBuilder();
clientBuilder.readTimeout(request.getReadTimeout());
clientBuilder.callTimeout(request.getTimeout());
}
if (request.isForStreaming()) {
if (clientBuilder == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ public CompletableFuture<WebSocketResponse> buildWebSocketDirect(StandardWebSock

final StandardHttpRequest request = standardWebSocketBuilder.asHttpRequest();

if (request.getReadTimeout() != null) {
options.setTimeout(request.getReadTimeout().toMillis());
if (request.getTimeout() != null) {
options.setTimeout(request.getTimeout().toMillis());
}

request.headers().entrySet().stream()
Expand Down Expand Up @@ -107,8 +107,8 @@ 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());
if (request.getTimeout() != null) {
options.setTimeout(request.getTimeout().toMillis());
}

// Proxy authorization is handled manually since the proxyAuthorization value is the actual header
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,11 @@ default Builder post(Map<String, String> formData) {
Builder expectContinue();

/**
* Sets the read timeout for normal http requests.
* Sets the full request cycle timeout.
* <br>
* Defaults to 0, meaning no timeout
*/
Builder readTimeout(long readTimeout, TimeUnit unit);
Builder timeout(long timeout, TimeUnit unit);

/**
* Sets the request to be used for streaming.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public CompletableFuture<HttpResponse<AsyncBody>> consumeBytes(HttpRequest reque

retryWithExponentialBackoff(result, () -> consumeBytesOnce(standardHttpRequest, consumer), request.uri(),
HttpResponse::code,
r -> r.body().cancel(), standardHttpRequest.getReadTimeout());
r -> r.body().cancel(), standardHttpRequest.getTimeout());
return result;
}

Expand Down Expand Up @@ -226,7 +226,7 @@ final CompletableFuture<WebSocket> buildWebSocket(StandardWebSocketBuilder stand
retryWithExponentialBackoff(intermediate, () -> buildWebSocketOnce(standardWebSocketBuilder, listener),
request.uri(),
r -> Optional.of(r.webSocketUpgradeResponse).map(HttpResponse::code).orElse(null),
r -> Optional.ofNullable(r.webSocket).ifPresent(w -> w.sendClose(1000, null)), request.getReadTimeout());
r -> Optional.ofNullable(r.webSocket).ifPresent(w -> w.sendClose(1000, null)), request.getTimeout());

CompletableFuture<WebSocket> result = new CompletableFuture<>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public long getLength() {
private final String bodyString;
private final BodyContent body;
private final boolean expectContinue;
private final Duration readTimeout;
private final Duration timeout;
private final boolean forStreaming;

/**
Expand All @@ -111,7 +111,7 @@ public StandardHttpRequest(Map<String, List<String>> headers, URI uri, String me
}

StandardHttpRequest(Map<String, List<String>> headers, URI uri, String method, String bodyString,
BodyContent body, boolean expectContinue, String contentType, Duration readTimeout, boolean forStreaming) {
BodyContent body, boolean expectContinue, String contentType, Duration timeout, boolean forStreaming) {
super(headers);
this.id = UUID.randomUUID();
this.uri = uri;
Expand All @@ -120,7 +120,7 @@ public StandardHttpRequest(Map<String, List<String>> headers, URI uri, String me
this.body = body;
this.expectContinue = expectContinue;
this.contentType = contentType;
this.readTimeout = readTimeout;
this.timeout = timeout;
this.forStreaming = forStreaming;
}

Expand Down Expand Up @@ -167,8 +167,8 @@ public boolean isForStreaming() {
return forStreaming;
}

public Duration getReadTimeout() {
return readTimeout;
public Duration getTimeout() {
return timeout;
}

public static final class Builder extends AbstractBasicBuilder<Builder> implements HttpRequest.Builder {
Expand All @@ -178,7 +178,7 @@ public static final class Builder extends AbstractBasicBuilder<Builder> implemen
private String bodyAsString;
private boolean expectContinue;
private String contentType;
protected Duration readTimeout;
protected Duration timeout;
protected boolean forStreaming;

public Builder() {
Expand All @@ -192,19 +192,19 @@ public Builder(StandardHttpRequest original) {
body = original.body;
expectContinue = original.expectContinue;
contentType = original.contentType;
readTimeout = original.readTimeout;
timeout = original.timeout;
forStreaming = original.forStreaming;
}

@Override
public StandardHttpRequest build() {
return new StandardHttpRequest(getHeaders(), Objects.requireNonNull(getUri()), method, bodyAsString, body, expectContinue,
contentType, readTimeout, forStreaming);
contentType, timeout, forStreaming);
}

@Override
public HttpRequest.Builder readTimeout(long readTimeout, TimeUnit unit) {
this.readTimeout = Duration.ofNanos(unit.toNanos(readTimeout));
public HttpRequest.Builder timeout(long timeout, TimeUnit unit) {
this.timeout = Duration.ofNanos(unit.toNanos(timeout));
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public StandardHttpRequest asHttpRequest() {

@Override
public Builder connectTimeout(long timeout, TimeUnit timeUnit) {
this.builder.readTimeout(timeout, timeUnit);
this.builder.timeout(timeout, timeUnit);
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ void testClosePreviousBeforeRetry() throws Exception {
@Test
void testRequestTimeout() throws Exception {
CompletableFuture<HttpResponse<AsyncBody>> consumeFuture = client.consumeBytes(
client.newHttpRequestBuilder().uri("http://localhost").readTimeout(1, TimeUnit.MILLISECONDS).build(),
client.newHttpRequestBuilder().uri("http://localhost").timeout(1, TimeUnit.MILLISECONDS).build(),
(value, asyncBody) -> {
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ protected <T> T handleRawGet(URL resourceUrl, Class<T> type) throws IOException
}

HttpRequest.Builder withReadTimeout(HttpRequest.Builder builder) {
return builder.readTimeout(getRequestConfig().getRequestTimeout(), TimeUnit.MILLISECONDS);
return builder.timeout(getRequestConfig().getRequestTimeout(), TimeUnit.MILLISECONDS);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import io.fabric8.kubernetes.api.model.EventList;
import io.fabric8.kubernetes.client.Client;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.RequestConfigBuilder;
import io.fabric8.kubernetes.client.dsl.Triggerable;
import io.fabric8.kubernetes.client.dsl.Typeable;
import io.fabric8.kubernetes.client.dsl.internal.HasMetadataOperation;
Expand Down Expand Up @@ -80,8 +79,7 @@ public class BuildConfigOperationsImpl
private final String asFile;

public BuildConfigOperationsImpl(Client client) {
this(new BuildConfigOperationContext(), HasMetadataOperationsImpl.defaultContext(client).withRequestConfig(
new RequestConfigBuilder(client.getConfiguration().getRequestConfig()).withRequestTimeout(0).build()));
this(new BuildConfigOperationContext(), HasMetadataOperationsImpl.defaultContext(client));
}

public BuildConfigOperationsImpl(BuildConfigOperationContext context, OperationContext superContext) {
Expand Down Expand Up @@ -241,10 +239,7 @@ public CommitterEmailable<AuthorMessageAsFileTimeoutInputStreamable<Build>> with
@Override
public BuildConfigOperationsImpl withTimeout(long timeout, TimeUnit unit) {
return new BuildConfigOperationsImpl(getContext(), context
.withTimeout(timeout, unit)
.withRequestConfig(new RequestConfigBuilder(context.getRequestConfig())
.withRequestTimeout((int) unit.toMillis(timeout))
.build()));
.withTimeout(timeout, unit));
}

@Override
Expand All @@ -262,7 +257,7 @@ protected Build submitToApiServer(InputStream inputStream, long contentLength) {
HttpRequest.Builder requestBuilder = this.httpClient.newHttpRequestBuilder()
.post("application/octet-stream", inputStream, contentLength)
.expectContinue()
.readTimeout(getOperationContext().getTimeout(), getOperationContext().getTimeoutUnit())
.timeout(getOperationContext().getTimeout(), getOperationContext().getTimeoutUnit())
.uri(getQueryParameters());
return waitForResult(handleResponse(this.httpClient, requestBuilder, new TypeReference<Build>() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public void setUp() {
when(httpClient.newHttpRequestBuilder()
.post(any(), any(), anyLong())
.header(any(), any())
.readTimeout(anyLong(), any())
.timeout(anyLong(), any())
.uri(any(String.class))
.build()).thenReturn(response);

Expand All @@ -71,23 +71,23 @@ public void setUp() {

@Test
void requestTimeoutDefaultsToZero() {
assertThat(new BuildConfigOperationsImpl(client).getOperationContext().getRequestConfig().getRequestTimeout())
assertThat(new BuildConfigOperationsImpl(client).getOperationContext().getTimeout())
.isZero();
}

@Test
void withTimeoutOverridesRequestTimeout() {
final BuildConfigOperationsImpl buildConfigOperations = new BuildConfigOperationsImpl(client)
.withTimeout(1337, TimeUnit.MILLISECONDS);
assertThat(buildConfigOperations.getOperationContext().getRequestConfig().getRequestTimeout())
assertThat(buildConfigOperations.getOperationContext().getTimeout())
.isEqualTo(1337);
}

@Test
void withTimeoutInMillisOverridesRequestTimeout() {
final BuildConfigOperationsImpl buildConfigOperations = new BuildConfigOperationsImpl(client)
.withTimeoutInMillis(1337);
assertThat(buildConfigOperations.getOperationContext().getRequestConfig().getRequestTimeout())
assertThat(buildConfigOperations.getOperationContext().getTimeout())
.isEqualTo(1337);
}

Expand Down

0 comments on commit 924d788

Please sign in to comment.