From 819b750da9f74ff885e086cbaa466519c67121f0 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Thu, 13 Sep 2018 13:02:36 +0100 Subject: [PATCH 01/15] Implement improved HttpClient interface in StyxHttpClient class, allowing per-request overrides. --- .../com/hotels/styx/client/HttpClient.java | 31 +- .../hotels/styx/client/StyxHttpClient.java | 202 +++++++++-- .../netty/connectionpool/NettyConnection.java | 22 +- .../NettyConnectionFactory.java | 21 +- .../styx/client/StyxHttpClientTest.java | 328 +++++++++++------- .../connectionpool/NettyConnectionTest.java | 4 +- 6 files changed, 438 insertions(+), 170 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java index d8429efdde..37620cefe2 100644 --- a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java @@ -17,6 +17,8 @@ import com.hotels.styx.api.FullHttpRequest; import com.hotels.styx.api.FullHttpResponse; +import com.hotels.styx.api.HttpRequest; +import com.hotels.styx.api.HttpResponse; import java.util.concurrent.CompletableFuture; @@ -24,15 +26,24 @@ * HTTP Client that returns an observable of response. */ public interface HttpClient { - /** - * Processes a new request. - * - * @param request HTTP request to process - * @return an observable of HTTP sendRequest. - * - * The sendRequest will be sent when the returned observable is subscribed to. - * In order to cancel the ongoing request, just unsubscribe from it. - * - */ +// CompletableFuture sendRequest(HttpRequest request); CompletableFuture sendRequest(FullHttpRequest request); + + interface Transaction { + Transaction secure(); + + Transaction secure(boolean secure); + + Transaction userAgent(String userAgent); + + StreamingTransaction streaming(); + +// CompletableFuture sendRequest(HttpRequest request); + CompletableFuture sendRequest(FullHttpRequest request); + } + + interface StreamingTransaction { + CompletableFuture sendRequest(HttpRequest request); + CompletableFuture sendRequest(FullHttpRequest request); + } } diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 5d74fee086..1d257a6806 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -24,6 +24,8 @@ import com.hotels.styx.api.extension.service.TlsSettings; import com.hotels.styx.client.netty.connectionpool.HttpRequestOperation; import com.hotels.styx.client.netty.connectionpool.NettyConnectionFactory; +import com.hotels.styx.client.ssl.SslContextFactory; +import io.netty.handler.ssl.SslContext; import rx.Observable; import java.util.Optional; @@ -44,39 +46,72 @@ public final class StyxHttpClient implements HttpClient { private static final int DEFAULT_HTTPS_PORT = 443; private static final int DEFAULT_HTTP_PORT = 80; - private final Optional userAgent; - private final ConnectionSettings connectionSettings; - private final int maxResponseSize; - private final Connection.Factory connectionFactory; - private final boolean isHttps; - - private StyxHttpClient(Builder builder, boolean isHttps) { - this.userAgent = Optional.ofNullable(builder.userAgent); - this.connectionSettings = builder.connectionSettings; - this.maxResponseSize = builder.maxResponseSize; - this.connectionFactory = builder.connectionFactory; - this.isHttps = isHttps; + private final ClientParameters clientParameters; + final NettyConnectionFactory connectionFactory; + + private StyxHttpClient(Builder builder) { + clientParameters = new ClientParameters(builder); + connectionFactory = new NettyConnectionFactory.Builder() + .name(clientParameters.threadName()) + .httpConfig(newHttpConfigBuilder().setMaxHeadersSize(clientParameters.maxHeaderSize()).build()) + .tlsSettings(clientParameters.tlsSettings().orElseGet(() -> clientParameters.https() ? new TlsSettings.Builder().build() : null)) + .httpRequestOperationFactory(aRequest -> new HttpRequestOperation( + aRequest, + null, + false, + clientParameters.responseTimeout(), + false, + false)) + .build(); + + } + + public HttpClient.Transaction secure() { + ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().secure(true)); + return new StyxHttpClientTransaction(connectionFactory, parameters); + } + + public HttpClient.Transaction secure(boolean secure) { + if (!secure && this.clientParameters.https()) { + throw new IllegalArgumentException("An insecure HTTP transaction is not allowed with HTTPS configuration."); + } + ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().secure(secure)); + return new StyxHttpClientTransaction(connectionFactory, parameters); + } + +// public HttpClient.Transaction secure(TlsSettings tlsSettings) { +// ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().tlsSettings(tlsSettings)); +// return new StyxHttpClientTransaction(connectionFactory, parameters); +// } + + public HttpClient.Transaction userAgent(String userAgent) { + ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().userAgent(userAgent)); + return new StyxHttpClientTransaction(connectionFactory, parameters); } public CompletableFuture sendRequest(FullHttpRequest request) { - FullHttpRequest networkRequest = addUserAgent(request); - Origin origin = originFromRequest(networkRequest, isHttps); + return sendRequestInternal(connectionFactory, request, this.clientParameters); + } + + private static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, ClientParameters parent) { + FullHttpRequest networkRequest = parent.addUserAgent(request); + Origin origin = originFromRequest(networkRequest, parent.https()); - Observable responseObservable = connectionFactory.createConnection(origin, connectionSettings) - .flatMap(connection -> connection.write(networkRequest.toStreamingRequest()) - .flatMap(response -> toRxObservable(response.toFullResponse(maxResponseSize))) - .doOnTerminate(connection::close)); + TlsSettings tlsSettings = parent.tlsSettings().orElseGet(() -> parent.https() ? new TlsSettings.Builder().build() : null); + + SslContext sslContext = Optional.ofNullable(tlsSettings) + .map(SslContextFactory::get) + .orElse(null); + + Observable responseObservable = connectionFactory.createConnection(origin, parent.connectionSettings(), sslContext) + .flatMap(connection -> + connection.write(networkRequest.toStreamingRequest()) + .flatMap(response -> toRxObservable(response.toFullResponse(parent.maxResponseSize()))) + .doOnTerminate(connection::close) + ); return fromSingleObservable(responseObservable); - } - private FullHttpRequest addUserAgent(FullHttpRequest request) { - return Optional.of(request) - .filter(req -> !req.header(USER_AGENT).isPresent()) - .flatMap(req -> userAgent.map(userAgent -> request.newBuilder() - .addHeader(USER_AGENT, userAgent) - .build())) - .orElse(request); } private static Origin originFromRequest(FullHttpRequest request, Boolean isHttps) { @@ -96,6 +131,113 @@ private static Origin originFromRequest(FullHttpRequest request, Boolean isHttps return newOriginBuilder(host.getHostText(), host.getPort()).build(); } + + static class ClientParameters { + private final ConnectionSettings connectionSettings; + private final Optional userAgent; + private final int maxResponseSize; + private final boolean isHttps; + private TlsSettings tlsSettings; + + private ClientParameters(Builder builder) { + this.userAgent = Optional.ofNullable(builder.userAgent); + this.connectionSettings = builder.connectionSettings; + this.maxResponseSize = builder.maxResponseSize; + this.isHttps = builder.isHttps; + this.tlsSettings = builder.tlsSettings; + } + + private Optional userAgent() { + return userAgent; + } + + private ConnectionSettings connectionSettings() { + return connectionSettings; + } + + private int maxResponseSize() { + return maxResponseSize; + } + + private boolean https() { + return isHttps; + } + + public String threadName() { + return "SimpleHttpClientThread"; + } + + private FullHttpRequest addUserAgent(FullHttpRequest request) { + return Optional.of(request) + .filter(req -> !req.header(USER_AGENT).isPresent()) + .flatMap(req -> userAgent.map(userAgent -> request.newBuilder() + .addHeader(USER_AGENT, userAgent) + .build())) + .orElse(request); + } + + public Builder newBuilder() { + Builder builder = new Builder() + .connectionSettings(connectionSettings) + .maxHeaderSize(maxResponseSize) + .tlsSettings(tlsSettings) + .secure(isHttps); + + userAgent.ifPresent(builder::userAgent); + + return builder; + } + + public int maxHeaderSize() { + return 8192; + } + + public Optional tlsSettings() { + return Optional.ofNullable(tlsSettings); + } + + public int responseTimeout() { + return 60000; + } + } + + static class StyxHttpClientTransaction implements HttpClient.Transaction { + private Builder builder; + private NettyConnectionFactory connectionFactory; + + public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, ClientParameters parent) { + this.builder = parent.newBuilder(); + this.connectionFactory = connectionFactory; + } + + @Override + public Transaction secure() { + this.builder.secure(true); + return this; + } + + @Override + public Transaction secure(boolean secure) { + this.builder.secure(secure); + return this; + } + + @Override + public Transaction userAgent(String userAgent) { + this.builder.userAgent(userAgent); + return this; } + + @Override + public StreamingTransaction streaming() { + return null; + } + + @Override + public CompletableFuture sendRequest(FullHttpRequest request) { + return StyxHttpClient.sendRequestInternal(connectionFactory, request, new ClientParameters(builder)); + } + } + /** * Builder for {@link StyxHttpClient}. */ @@ -108,6 +250,7 @@ public static class Builder { private int maxResponseSize = 1024 * 100; private int maxHeaderSize = 8192; private String threadName = "simple-netty-http-client"; + private boolean isHttps; public Builder connectionSettings(ConnectionSettings connectionSettings) { this.connectionSettings = connectionSettings; @@ -137,9 +280,14 @@ public Builder maxResponseSize(int maxResponseSize) { public Builder tlsSettings(TlsSettings tlsSettings) { this.tlsSettings = tlsSettings; + this.isHttps = tlsSettings != null; return this; } + public Builder secure(boolean secure) { + this.isHttps = secure; return this; + } + public Builder maxHeaderSize(int maxHeaderSize) { this.maxHeaderSize = maxHeaderSize; return this; @@ -174,7 +322,7 @@ public StyxHttpClient build() { false, false)) .build(); - return new StyxHttpClient(this, tlsSettings != null); + return new StyxHttpClient(this); } } } diff --git a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java index 949513637b..caa0df8cb9 100644 --- a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java +++ b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java @@ -21,9 +21,14 @@ import com.hotels.styx.api.HttpResponse; import com.hotels.styx.client.Connection; import com.hotels.styx.api.extension.Origin; +import com.hotels.styx.client.HttpConfig; import com.hotels.styx.client.HttpRequestOperationFactory; import io.netty.channel.Channel; +import io.netty.channel.ChannelPipeline; +import io.netty.handler.codec.http.HttpClientCodec; +import io.netty.handler.codec.http.HttpContentDecompressor; +import io.netty.handler.ssl.SslContext; import io.netty.util.AttributeKey; import rx.Observable; @@ -54,13 +59,28 @@ public class NettyConnection implements Connection, TimeToFirstByteListener { * @param requestOperationFactory used to create operation objects that send http requests via this connection */ @VisibleForTesting - public NettyConnection(Origin origin, Channel channel, HttpRequestOperationFactory requestOperationFactory) { + public NettyConnection(Origin origin, Channel channel, HttpRequestOperationFactory requestOperationFactory, HttpConfig httpConfig, SslContext sslContext) { this.origin = requireNonNull(origin); this.channel = requireNonNull(channel); this.requestOperationFactory = requestOperationFactory; this.channel.pipeline().addLast(new TimeToFirstByteHandler(this)); this.channel.closeFuture().addListener(future -> listeners.announce().connectionClosed(NettyConnection.this)); + + addChannelHandlers(channel, httpConfig, sslContext); + } + + private static void addChannelHandlers(Channel channel, HttpConfig httpConfig, SslContext sslContext) { + ChannelPipeline pipeline = channel.pipeline(); + + if (sslContext != null) { + pipeline.addLast("ssl", sslContext.newHandler(channel.alloc())); + } + + pipeline.addLast("http-codec", new HttpClientCodec(httpConfig.maxInitialLineLength(), httpConfig.maxHeadersSize(), httpConfig.maxChunkSize())); + if (httpConfig.compress()) { + pipeline.addLast("decompressor", new HttpContentDecompressor()); + } } @Override diff --git a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java index d2e0dcfa5d..a25e9f367e 100644 --- a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java +++ b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java @@ -18,12 +18,12 @@ import com.hotels.styx.api.exceptions.OriginUnreachableException; import com.hotels.styx.api.extension.Origin; import com.hotels.styx.api.extension.service.TlsSettings; -import com.hotels.styx.client.netty.ClientEventLoopFactory; import com.hotels.styx.client.ChannelOptionSetting; import com.hotels.styx.client.Connection; import com.hotels.styx.client.ConnectionSettings; import com.hotels.styx.client.HttpConfig; import com.hotels.styx.client.HttpRequestOperationFactory; +import com.hotels.styx.client.netty.ClientEventLoopFactory; import com.hotels.styx.client.netty.eventloop.PlatformAwareClientEventLoopGroupFactory; import com.hotels.styx.client.ssl.SslContextFactory; import io.netty.bootstrap.Bootstrap; @@ -31,9 +31,6 @@ import io.netty.channel.Channel; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelInitializer; -import io.netty.channel.ChannelPipeline; -import io.netty.handler.codec.http.HttpClientCodec; -import io.netty.handler.codec.http.HttpContentDecompressor; import io.netty.handler.ssl.SslContext; import rx.Observable; @@ -65,12 +62,16 @@ private NettyConnectionFactory(Builder builder) { @Override public Observable createConnection(Origin origin, ConnectionSettings connectionSettings) { + return createConnection(origin, connectionSettings, sslContext); + } + + public Observable createConnection(Origin origin, ConnectionSettings connectionSettings, SslContext sslContext) { return Observable.create(subscriber -> { ChannelFuture channelFuture = openConnection(origin, connectionSettings); channelFuture.addListener(future -> { if (future.isSuccess()) { - subscriber.onNext(new NettyConnection(origin, channelFuture.channel(), httpRequestOperationFactory)); + subscriber.onNext(new NettyConnection(origin, channelFuture.channel(), httpRequestOperationFactory, httpConfig, sslContext)); subscriber.onCompleted(); } else { subscriber.onError(new OriginUnreachableException(origin, future.cause())); @@ -103,16 +104,6 @@ private synchronized void bootstrap(ConnectionSettings connectionSettings) { private class Initializer extends ChannelInitializer { @Override protected void initChannel(Channel ch) { - ChannelPipeline pipeline = ch.pipeline(); - - if (sslContext != null) { - pipeline.addLast("ssl", sslContext.newHandler(ch.alloc())); - } - - pipeline.addLast("http-codec", new HttpClientCodec(httpConfig.maxInitialLineLength(), httpConfig.maxHeadersSize(), httpConfig.maxChunkSize())); - if (httpConfig.compress()) { - pipeline.addLast("decompressor", new HttpContentDecompressor()); - } } } diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index a86b21cb30..094c876d9c 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -21,190 +21,299 @@ import com.hotels.styx.api.FullHttpResponse; import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; -import com.hotels.styx.api.extension.Origin; import com.hotels.styx.api.exceptions.OriginUnreachableException; +import com.hotels.styx.api.extension.Origin; import com.hotels.styx.api.extension.service.TlsSettings; -import org.mockito.ArgumentCaptor; +import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import rx.Observable; import java.io.IOException; import java.util.concurrent.ExecutionException; -import java.util.function.IntConsumer; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; import static com.hotels.styx.api.FullHttpRequest.get; import static com.hotels.styx.api.HttpHeaderNames.HOST; -import static com.hotels.styx.api.HttpHeaderNames.USER_AGENT; -import static com.hotels.styx.api.HttpResponse.response; import static com.hotels.styx.api.HttpResponseStatus.OK; -import static com.hotels.styx.client.Protocol.HTTP; -import static com.hotels.styx.client.Protocol.HTTPS; import static com.hotels.styx.common.StyxFutures.await; -import static com.hotels.styx.support.matchers.IsOptional.isAbsent; -import static com.hotels.styx.support.matchers.IsOptional.isValue; import static com.hotels.styx.support.server.UrlMatchingStrategies.urlStartingWith; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class StyxHttpClientTest { private FullHttpRequest anyRequest; private static int MAX_LENGTH = 1024; + private WireMockServer server; @BeforeMethod public void setUp() { + server = new WireMockServer(wireMockConfig().dynamicPort().dynamicHttpsPort()); + server.start(); + server.stubFor(WireMock.get(urlStartingWith("/")).willReturn(aResponse().withStatus(200))); + anyRequest = get("/foo.txt") .header(HOST, "localhost:1234") .build(); } - @Test - public void sendsHttp() { - withOrigin(HTTP, port -> { - FullHttpResponse response = await(httpClient().sendRequest(httpRequest(port))); - assertThat(response.status(), is(OK)); - }); + + @AfterMethod + public void tearDown() { + server.stop(); } @Test - public void sendsHttps() { - withOrigin(HTTPS, port -> { - FullHttpResponse response = await(httpsClient().sendRequest(httpsRequest(port))); - assertThat(response.status(), is(OK)); - }); - } + public void usesDefaultUserAgent() throws ExecutionException, InterruptedException { + StyxHttpClient client = new StyxHttpClient.Builder() + .userAgent("Simple-Client-Parent-Settings") + .build(); - @Test(expectedExceptions = OriginUnreachableException.class) - public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Throwable { - try { - httpClient().sendRequest(get("/foo.txt").header(HOST, "a.b.c").build()).get(); - } catch (ExecutionException cause) { - throw cause.getCause(); - } - } + FullHttpResponse response = client + .sendRequest(httpRequest(server.port())) + .get(); - @Test(expectedExceptions = Exception.class) - public void cannotSendHttpsWhenConfiguredForHttp() { - withOrigin(HTTPS, port -> { - FullHttpResponse response = await(httpClient().sendRequest(httpsRequest(port))); - assertThat(response.status(), is(OK)); - }); + assertThat(response.status(), is(OK)); + server.verify( + getRequestedFor(urlEqualTo("/")) + .withHeader("User-Agent", equalTo("Simple-Client-Parent-Settings")) + ); } - @Test(expectedExceptions = Exception.class) - public void cannotSendHttpWhenConfiguredForHttps() throws IOException { - withOrigin(HTTP, port -> { - FullHttpResponse response = await(httpsClient().sendRequest(httpRequest(port))); - assertThat(response.status(), is(OK)); - }); + @Test + public void usesPerTransactionUserAgent() throws ExecutionException, InterruptedException { + StyxHttpClient client = new StyxHttpClient.Builder() + .userAgent("Simple-Client-Parent-Settings") + .build(); + + FullHttpResponse response = client + .userAgent("Simple-Client-Transaction-Settings") + .sendRequest(httpRequest(server.port())) + .get(); + + assertThat(response.status(), is(OK)); + server.verify( + getRequestedFor(urlEqualTo("/")) + .withHeader("User-Agent", equalTo("Simple-Client-Transaction-Settings")) + ); } @Test - public void willNotSetAnyUserAgentIfNotSpecified() { - Connection mockConnection = mock(Connection.class); - when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); - + public void usesDefaultTlsSettings() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() - .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) + .tlsSettings(new TlsSettings.Builder().build()) .build(); - client.sendRequest(anyRequest); + FullHttpResponse response = client + .sendRequest(httpRequest(server.httpsPort())) + .get(); - ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); - verify(mockConnection).write(captor.capture()); - assertThat(captor.getValue().header(USER_AGENT), isAbsent()); + assertThat(response.status(), is(OK)); } - @Test - public void setsTheSpecifiedUserAgent() { - Connection mockConnection = mock(Connection.class); - when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); +// @Test +// public void overridesTlsSettings() throws ExecutionException, InterruptedException { +// StyxHttpClient client = new StyxHttpClient.Builder() +// .build(); +// +// FullHttpResponse response = client +// .secure(new TlsSettings.Builder().build()) +// .sendRequest(httpRequest(server.httpsPort())) +// .get(); +// +// assertThat(response.status(), is(OK)); +// } + @Test + public void overridesTlsSettingsWithSecure() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() - .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) - .userAgent("Styx/5.6") .build(); - client.sendRequest(anyRequest); + FullHttpResponse response = client + .secure() + .sendRequest(httpRequest(server.httpsPort())) + .get(); - ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); - verify(mockConnection).write(captor.capture()); - assertThat(captor.getValue().header(USER_AGENT), isValue("Styx/5.6")); + assertThat(response.status(), is(OK)); } @Test - public void retainsTheUserAgentStringFromTheRequest() { - Connection mockConnection = mock(Connection.class); - when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); - + public void overridesTlsSettingsWithSecureBoolean() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() - .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) - .userAgent("Styx/5.6") .build(); - client.sendRequest(get("/foo.txt") - .header(USER_AGENT, "Foo/Bar") - .header(HOST, "localhost:1234") - .build()); + FullHttpResponse response = client + .secure(true) + .sendRequest(httpRequest(server.httpsPort())) + .get(); - ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); - verify(mockConnection).write(captor.capture()); - assertThat(captor.getValue().header(USER_AGENT), isValue("Foo/Bar")); + assertThat(response.status(), is(OK)); } - @Test(expectedExceptions = IllegalArgumentException.class) - public void requestWithNoHostOrUrlAuthorityCausesException() { - FullHttpRequest request = get("/foo.txt").build(); - - StyxHttpClient client = new StyxHttpClient.Builder().build(); + @Test(expectedExceptions = IllegalArgumentException.class, + expectedExceptionsMessageRegExp = "An insecure HTTP transaction is not allowed with HTTPS configuration.") + public void canNotDowngradeDefaultSecureSettings() { + StyxHttpClient client = new StyxHttpClient.Builder() + .tlsSettings(new TlsSettings.Builder().build()) + .build(); - await(client.sendRequest(request)); + client.secure(false); } @Test - public void sendsToDefaultHttpPort() { - Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); - + public void disablesTlsIfNullArgumentIsPassed() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() - .setConnectionFactory(connectionFactory) + .tlsSettings(null) .build(); - client.sendRequest(get("/") - .header(HOST, "localhost") - .build()); + FullHttpResponse response = client + .secure(false) + .sendRequest(httpRequest(server.port())) + .get(); - ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); - verify(connectionFactory).createConnection(originCaptor.capture(), any(ConnectionSettings.class)); + assertThat(response.status(), is(OK)); + } - assertThat(originCaptor.getValue().port(), is(80)); + @Test + public void sendsHttp() { + FullHttpResponse response = await(httpClient().sendRequest(httpRequest(server.port()))); + assertThat(response.status(), is(OK)); } @Test - public void sendsToDefaultHttpsPort() { - Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); - TlsSettings tlsSettings = mock(TlsSettings.class); + public void sendsHttps() { + FullHttpResponse response = await(httpsClient().sendRequest(httpsRequest(server.httpsPort()))); + assertThat(response.status(), is(OK)); + } - StyxHttpClient client = new StyxHttpClient.Builder() - .setConnectionFactory(connectionFactory) - .tlsSettings(tlsSettings) - .build(); + @Test(expectedExceptions = OriginUnreachableException.class) + public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Throwable { + try { + httpClient().sendRequest(get("/foo.txt").header(HOST, "a.b.c").build()).get(); + } catch (ExecutionException cause) { + throw cause.getCause(); + } + } + + @Test(expectedExceptions = Exception.class) + public void cannotSendHttpsWhenConfiguredForHttp() { + FullHttpResponse response = await(httpClient().sendRequest(httpsRequest(server.httpsPort()))); + assertThat(response.status(), is(OK)); + } + + @Test(expectedExceptions = Exception.class) + public void cannotSendHttpWhenConfiguredForHttps() throws IOException { + FullHttpResponse response = await(httpsClient().sendRequest(httpRequest(server.port()))); + assertThat(response.status(), is(OK)); + } - client.sendRequest(get("/") - .header(HOST, "localhost") - .build()); +// @Test +// public void willNotSetAnyUserAgentIfNotSpecified() { +// Connection mockConnection = mock(Connection.class); +// when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); +// +// StyxHttpClient client = new StyxHttpClient.Builder() +// .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) +// .build(); +// +// client.sendRequest(anyRequest); +// +// ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); +// verify(mockConnection).write(captor.capture()); +// assertThat(captor.getValue().header(USER_AGENT), isAbsent()); +// } +// +// @Test +// public void setsTheSpecifiedUserAgent() { +// Connection mockConnection = mock(Connection.class); +// when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); +// +// StyxHttpClient client = new StyxHttpClient.Builder() +// .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) +// .userAgent("Styx/5.6") +// .build(); +// +// client.sendRequest(anyRequest); +// +// ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); +// verify(mockConnection).write(captor.capture()); +// assertThat(captor.getValue().header(USER_AGENT), isValue("Styx/5.6")); +// } +// +// @Test +// public void retainsTheUserAgentStringFromTheRequest() { +// Connection mockConnection = mock(Connection.class); +// when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); +// +// StyxHttpClient client = new StyxHttpClient.Builder() +// .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) +// .userAgent("Styx/5.6") +// .build(); +// +// client.sendRequest(get("/foo.txt") +// .header(USER_AGENT, "Foo/Bar") +// .header(HOST, "localhost:1234") +// .build()); +// +// ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); +// verify(mockConnection).write(captor.capture()); +// assertThat(captor.getValue().header(USER_AGENT), isValue("Foo/Bar")); +// } - ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); - verify(connectionFactory).createConnection(originCaptor.capture(), any(ConnectionSettings.class)); + @Test(expectedExceptions = IllegalArgumentException.class) + public void requestWithNoHostOrUrlAuthorityCausesException() { + FullHttpRequest request = get("/foo.txt").build(); - assertThat(originCaptor.getValue().port(), is(443)); + StyxHttpClient client = new StyxHttpClient.Builder().build(); + + await(client.sendRequest(request)); } +// @Test +// public void sendsToDefaultHttpPort() { +// Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); +// +// StyxHttpClient client = new StyxHttpClient.Builder() +// .setConnectionFactory(connectionFactory) +// .build(); +// +// client.sendRequest(get("/") +// .header(HOST, "localhost") +// .build()); +// +// ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); +// verify(connectionFactory).createConnection(originCaptor.capture(), any(ConnectionSettings.class)); +// +// assertThat(originCaptor.getValue().port(), is(80)); +// } +// +// @Test +// public void sendsToDefaultHttpsPort() { +// Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); +// TlsSettings tlsSettings = mock(TlsSettings.class); +// +// StyxHttpClient client = new StyxHttpClient.Builder() +// .setConnectionFactory(connectionFactory) +// .tlsSettings(tlsSettings) +// .build(); +// +// client.sendRequest(get("/") +// .header(HOST, "localhost") +// .build()); +// +// ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); +// verify(connectionFactory).createConnection(originCaptor.capture(), any(ConnectionSettings.class)); +// +// assertThat(originCaptor.getValue().port(), is(443)); +// } + private StyxHttpClient httpClient() { return new StyxHttpClient.Builder() .build(); @@ -232,19 +341,6 @@ private FullHttpRequest httpsRequest(int port) { .build(); } - private void withOrigin(Protocol protocol, IntConsumer portConsumer) { - WireMockServer server = new WireMockServer(wireMockConfig().dynamicPort().dynamicHttpsPort()); - - try { - server.start(); - int port = protocol == HTTP ? server.port() : server.httpsPort(); - server.stubFor(WireMock.get(urlStartingWith("/")).willReturn(aResponse().withStatus(200))); - portConsumer.accept(port); - } finally { - server.stop(); - } - } - private Connection mockConnection(HttpResponse response) { Connection connection = mock(Connection.class); when(connection.write(any(HttpRequest.class))).thenReturn(Observable.just(response)); diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionTest.java index 96ecc2031a..bbc9e44fa4 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionTest.java @@ -17,6 +17,7 @@ import com.hotels.styx.client.Connection; import com.hotels.styx.api.extension.Origin; +import com.hotels.styx.client.HttpConfig; import io.netty.channel.Channel; import io.netty.channel.embedded.EmbeddedChannel; import org.testng.annotations.BeforeMethod; @@ -34,6 +35,7 @@ public class NettyConnectionTest { private Channel channel; private Origin origin; + private HttpConfig httpConfig = HttpConfig.defaultHttpConfig(); @BeforeMethod public void startOriginServer() { @@ -64,7 +66,7 @@ public void notifiesListenersWhenConnectionIsClosed() throws Exception { } private Connection createConnection() { - return new NettyConnection(origin, channel, null); + return new NettyConnection(origin, channel, null, httpConfig, null); } static class EventCapturingListener implements Connection.Listener { From 33c2a2935e8a3a4c69d3d29c8607c866d01ecbef Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Fri, 14 Sep 2018 09:16:29 +0100 Subject: [PATCH 02/15] Implement shutdown() method for StyxHttpClient. --- .../hotels/styx/client/StyxHttpClient.java | 51 ++++++------------- .../NettyConnectionFactory.java | 32 +++++++++--- .../styx/client/StyxHttpClientTest.java | 40 +++++++++------ .../styx/proxy/BackendServicesRouter.java | 2 +- .../com/hotels/styx/StyxClientSupplier.scala | 4 +- .../com/hotels/styx/proxy/ProxySpec.scala | 2 +- 6 files changed, 71 insertions(+), 60 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 1d257a6806..48c5efba4e 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -15,7 +15,6 @@ */ package com.hotels.styx.client; -import com.google.common.annotations.VisibleForTesting; import com.google.common.net.HostAndPort; import com.hotels.styx.api.FullHttpRequest; import com.hotels.styx.api.FullHttpResponse; @@ -51,19 +50,11 @@ public final class StyxHttpClient implements HttpClient { private StyxHttpClient(Builder builder) { clientParameters = new ClientParameters(builder); - connectionFactory = new NettyConnectionFactory.Builder() - .name(clientParameters.threadName()) - .httpConfig(newHttpConfigBuilder().setMaxHeadersSize(clientParameters.maxHeaderSize()).build()) - .tlsSettings(clientParameters.tlsSettings().orElseGet(() -> clientParameters.https() ? new TlsSettings.Builder().build() : null)) - .httpRequestOperationFactory(aRequest -> new HttpRequestOperation( - aRequest, - null, - false, - clientParameters.responseTimeout(), - false, - false)) - .build(); + connectionFactory = builder.connectionFactory; + } + public CompletableFuture shutdown() { + return connectionFactory.close(); } public HttpClient.Transaction secure() { @@ -79,11 +70,6 @@ public HttpClient.Transaction secure(boolean secure) { return new StyxHttpClientTransaction(connectionFactory, parameters); } -// public HttpClient.Transaction secure(TlsSettings tlsSettings) { -// ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().tlsSettings(tlsSettings)); -// return new StyxHttpClientTransaction(connectionFactory, parameters); -// } - public HttpClient.Transaction userAgent(String userAgent) { ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().userAgent(userAgent)); return new StyxHttpClientTransaction(connectionFactory, parameters); @@ -178,7 +164,7 @@ private FullHttpRequest addUserAgent(FullHttpRequest request) { public Builder newBuilder() { Builder builder = new Builder() - .connectionSettings(connectionSettings) + .connectTimeout(connectionSettings.connectTimeoutMillis()) .maxHeaderSize(maxResponseSize) .tlsSettings(tlsSettings) .secure(isHttps); @@ -242,7 +228,7 @@ public CompletableFuture sendRequest(FullHttpRequest request) * Builder for {@link StyxHttpClient}. */ public static class Builder { - private Connection.Factory connectionFactory; + private NettyConnectionFactory connectionFactory; private TlsSettings tlsSettings; private String userAgent; private ConnectionSettings connectionSettings = new ConnectionSettings(1000); @@ -252,8 +238,13 @@ public static class Builder { private String threadName = "simple-netty-http-client"; private boolean isHttps; - public Builder connectionSettings(ConnectionSettings connectionSettings) { - this.connectionSettings = connectionSettings; + public Builder connectTimeout(int timeoutMs) { + this.connectionSettings = new ConnectionSettings(timeoutMs); + return this; + } + + public Builder responseTimeout(int responseTimeoutMs) { + this.responseTimeout = responseTimeoutMs; return this; } @@ -268,11 +259,6 @@ public Builder userAgent(String userAgent) { return this; } - public Builder responseTimeoutMillis(int responseTimeout) { - this.responseTimeout = responseTimeout; - return this; - } - public Builder maxResponseSize(int maxResponseSize) { this.maxResponseSize = maxResponseSize; return this; @@ -285,7 +271,8 @@ public Builder tlsSettings(TlsSettings tlsSettings) { } public Builder secure(boolean secure) { - this.isHttps = secure; return this; + this.isHttps = secure; + return this; } public Builder maxHeaderSize(int maxHeaderSize) { @@ -298,19 +285,13 @@ public Builder threadName(String threadName) { return this; } - @VisibleForTesting - Builder setConnectionFactory(Connection.Factory connectionFactory) { - this.connectionFactory = connectionFactory; - return this; - } - /** * Construct a client instance. * * @return a new instance */ public StyxHttpClient build() { - connectionFactory = connectionFactory != null ? connectionFactory : new NettyConnectionFactory.Builder() + connectionFactory = new NettyConnectionFactory.Builder() .name(threadName) .httpConfig(newHttpConfigBuilder().setMaxHeadersSize(maxHeaderSize).build()) .tlsSettings(tlsSettings) diff --git a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java index a25e9f367e..19113c1bd0 100644 --- a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java +++ b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java @@ -23,7 +23,6 @@ import com.hotels.styx.client.ConnectionSettings; import com.hotels.styx.client.HttpConfig; import com.hotels.styx.client.HttpRequestOperationFactory; -import com.hotels.styx.client.netty.ClientEventLoopFactory; import com.hotels.styx.client.netty.eventloop.PlatformAwareClientEventLoopGroupFactory; import com.hotels.styx.client.ssl.SslContextFactory; import io.netty.bootstrap.Bootstrap; @@ -31,9 +30,12 @@ import io.netty.channel.Channel; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelInitializer; +import io.netty.channel.EventLoopGroup; import io.netty.handler.ssl.SslContext; import rx.Observable; +import java.util.concurrent.CompletableFuture; + import static com.hotels.styx.client.HttpConfig.defaultHttpConfig; import static com.hotels.styx.client.HttpRequestOperationFactory.Builder.httpRequestOperationFactoryBuilder; import static io.netty.channel.ChannelOption.ALLOCATOR; @@ -46,17 +48,22 @@ * A connection factory that creates connections using netty. */ public class NettyConnectionFactory implements Connection.Factory { - private final ClientEventLoopFactory eventLoopFactory; private final HttpConfig httpConfig; private final SslContext sslContext; private final HttpRequestOperationFactory httpRequestOperationFactory; private Bootstrap bootstrap; + private EventLoopGroup eventLoopGroup; + private Class clientSocketChannelClass; private NettyConnectionFactory(Builder builder) { - this.eventLoopFactory = new PlatformAwareClientEventLoopGroupFactory(builder.name, builder.clientWorkerThreadsCount); + PlatformAwareClientEventLoopGroupFactory eventLoopGroupFactory = new PlatformAwareClientEventLoopGroupFactory( + builder.name, + builder.clientWorkerThreadsCount + ); + this.eventLoopGroup = eventLoopGroupFactory.newClientWorkerEventLoopGroup(); this.httpConfig = requireNonNull(builder.httpConfig); this.sslContext = builder.tlsSettings == null ? null : SslContextFactory.get(builder.tlsSettings); - + this.clientSocketChannelClass = eventLoopGroupFactory.clientSocketChannelClass(); this.httpRequestOperationFactory = requireNonNull(builder.httpRequestOperationFactory); } @@ -88,8 +95,8 @@ private ChannelFuture openConnection(Origin origin, ConnectionSettings connectio private synchronized void bootstrap(ConnectionSettings connectionSettings) { if (bootstrap == null) { bootstrap = new Bootstrap(); - bootstrap.group(eventLoopFactory.newClientWorkerEventLoopGroup()) - .channel(eventLoopFactory.clientSocketChannelClass()) + bootstrap.group(eventLoopGroup) + .channel(clientSocketChannelClass) .handler(new Initializer()) .option(TCP_NODELAY, true) .option(SO_KEEPALIVE, true) @@ -101,6 +108,19 @@ private synchronized void bootstrap(ConnectionSettings connectionSettings) { } } + public CompletableFuture close() { + CompletableFuture future = new CompletableFuture(); + eventLoopGroup.shutdownGracefully() + .addListener(nettyFuture -> { + if (nettyFuture.isSuccess()) { + future.complete(null); + } else { + future.completeExceptionally(nettyFuture.cause()); + } + }); + return future; + } + private class Initializer extends ChannelInitializer { @Override protected void initChannel(Channel ch) { diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index 094c876d9c..d2248bfef2 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -70,6 +70,29 @@ public void tearDown() { server.stop(); } + @Test + public void closesThreadpoolAfterUse() throws InterruptedException, ExecutionException { + StyxHttpClient client = new StyxHttpClient.Builder() + .threadName("test-client") + .build(); + + // Ensures that thread is created + client.sendRequest(httpRequest(server.port())).get(); + + assertThat(threadExists("test-client"), is(true)); + + client.shutdown().get(); + + assertThat(threadExists("test-client"), is(false)); + } + + private static Boolean threadExists(String threadName) { + for (Thread t : Thread.getAllStackTraces().keySet()) { + if (t.getName().startsWith(threadName)) return true; + } + return false; + } + @Test public void usesDefaultUserAgent() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() @@ -118,19 +141,6 @@ public void usesDefaultTlsSettings() throws ExecutionException, InterruptedExcep assertThat(response.status(), is(OK)); } -// @Test -// public void overridesTlsSettings() throws ExecutionException, InterruptedException { -// StyxHttpClient client = new StyxHttpClient.Builder() -// .build(); -// -// FullHttpResponse response = client -// .secure(new TlsSettings.Builder().build()) -// .sendRequest(httpRequest(server.httpsPort())) -// .get(); -// -// assertThat(response.status(), is(OK)); -// } - @Test public void overridesTlsSettingsWithSecure() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() @@ -321,11 +331,11 @@ private StyxHttpClient httpClient() { private StyxHttpClient httpsClient() { return new StyxHttpClient.Builder() - .connectionSettings(new ConnectionSettings(1000)) + .connectTimeout(1000) .tlsSettings(new TlsSettings.Builder() .authenticate(false) .build()) - .responseTimeoutMillis(6000) + .responseTimeout(6000) .build(); } diff --git a/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java b/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java index df397c1d90..e8f1d1f30a 100644 --- a/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java +++ b/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java @@ -206,7 +206,7 @@ private static OriginHealthCheckFunction originHealthCheckFunction( connectionPoolSettings.connectTimeoutMillis()); StyxHttpClient client = new StyxHttpClient.Builder() - .connectionSettings(connectionSettings) + .connectTimeout(connectionSettings.connectTimeoutMillis()) .threadName("Health-Check-Monitor-" + appId) .userAgent("Styx/" + styxVersion) .tlsSettings(tlsSettings.orElse(null)) diff --git a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala index 364724c31b..7a0985da87 100644 --- a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala +++ b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala @@ -30,13 +30,13 @@ trait StyxClientSupplier { val client: HttpClient = new StyxHttpClient.Builder() .threadName("scalatest-e2e-client") - .connectionSettings(new ConnectionSettings(1000)) + .connectTimeout(1000) .maxHeaderSize(2 * 8192) .build() val httpsClient: HttpClient = new StyxHttpClient.Builder() .threadName("scalatest-e2e-secure-client") - .connectionSettings(new ConnectionSettings(1000)) + .connectTimeout(1000) .maxHeaderSize(2 * 8192) .tlsSettings(new TlsSettings.Builder().build()) .build() diff --git a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala index 8284403cac..ea7887d674 100644 --- a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala +++ b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala @@ -124,7 +124,7 @@ class ProxySpec extends FunSpec it("should respond to HEAD with bodiless response") { val client: HttpClient = new StyxHttpClient.Builder() - .connectionSettings(new ConnectionSettings(1000)) + .connectTimeout(1000) .maxHeaderSize(2 * 8192) .threadName("scalatest-e2e-client") .build() From 955a082b17b6e6a6ee5e652b77433fb5957b3102 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Fri, 14 Sep 2018 19:11:35 +0100 Subject: [PATCH 03/15] Improve HttpClient API docs. Remove userAgent from HttpClient.Transaction. Tidy up e2e client supplier. --- .../com/hotels/styx/client/HttpClient.java | 59 ++++++++++++++++++- .../hotels/styx/client/StyxHttpClient.java | 5 -- .../styx/client/StyxHttpClientTest.java | 16 ++--- .../com/hotels/styx/StyxClientSupplier.scala | 19 ++---- 4 files changed, 69 insertions(+), 30 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java index 37620cefe2..2e9eb19684 100644 --- a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java @@ -23,25 +23,78 @@ import java.util.concurrent.CompletableFuture; /** - * HTTP Client that returns an observable of response. + * A Styx HTTP client interface. + * + * This interface offers a fluent interface to build and configure HTTP + * request transactions from a client instance. The requests can be consumed + * either aggregated {@link FullHttpResponse} or streaming {@link HttpRequest} + * messages. */ public interface HttpClient { // CompletableFuture sendRequest(HttpRequest request); + + + /** + * Sends a HTTP request message using this client. + * + * @param request a full HTTP request object + * @return a future of full HTTP request object + */ CompletableFuture sendRequest(FullHttpRequest request); + /** + * A HTTP request transaction. + * + * This interface allows client attributes and context to be customised + * for each request without having to rely on configured default values + * in the client. + * + */ interface Transaction { + /** + * Send the request using TLS protocol. + * + * @return this @{code Transaction} object + */ Transaction secure(); + /** + * Determines if the request should be sent securely or not. + * + * @param secure Set to @{code true} if the request should be sent securely, + * or @{code false} if the request should be sent insecurely. + * @return this @{code Transaction} object + */ Transaction secure(boolean secure); - Transaction userAgent(String userAgent); - + /** + * Converts the transaction object to streaming transaction. + * + * A call to {@code streaming()} converts this {@link Transaction} object to + * a @{link StreamingTransaction}. This allows responses to be consumed + * in streaming responses. + * + * @return a {@link StreamingTransaction} object + */ StreamingTransaction streaming(); // CompletableFuture sendRequest(HttpRequest request); + + /** + * Sends a HTTP request message using this client. + * + * @param request a full HTTP request object + * @return a future of full HTTP request object + */ CompletableFuture sendRequest(FullHttpRequest request); } + /** + * A streaming HTTP request transaction. + * + * This interface allows the response object to be consumed in a streaming + * fashion instead of being aggregated into a FullHttpResponse. + */ interface StreamingTransaction { CompletableFuture sendRequest(HttpRequest request); CompletableFuture sendRequest(FullHttpRequest request); diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 48c5efba4e..3457c6a6b0 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -208,11 +208,6 @@ public Transaction secure(boolean secure) { return this; } - @Override - public Transaction userAgent(String userAgent) { - this.builder.userAgent(userAgent); - return this; } - @Override public StreamingTransaction streaming() { return null; diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index d2248bfef2..edb8f4f078 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -203,14 +203,14 @@ public void sendsHttps() { assertThat(response.status(), is(OK)); } - @Test(expectedExceptions = OriginUnreachableException.class) - public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Throwable { - try { - httpClient().sendRequest(get("/foo.txt").header(HOST, "a.b.c").build()).get(); - } catch (ExecutionException cause) { - throw cause.getCause(); - } - } +// @Test(expectedExceptions = OriginUnreachableException.class) +// public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Throwable { +// try { +// httpClient().sendRequest(get("/foo.txt").header(HOST, "a.b.c").build()).get(); +// } catch (ExecutionException cause) { +// throw cause.getCause(); +// } +// } @Test(expectedExceptions = Exception.class) public void cannotSendHttpsWhenConfiguredForHttp() { diff --git a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala index 7a0985da87..97c9262cb4 100644 --- a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala +++ b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala @@ -16,35 +16,26 @@ package com.hotels.styx import com.hotels.styx.api._ -import com.hotels.styx.api.extension.service.TlsSettings -import com.hotels.styx.client.{ConnectionSettings, HttpClient, StyxHttpClient} +import com.hotels.styx.client.StyxHttpClient -import scala.concurrent.{Await, Future} -import scala.concurrent.duration._ import scala.compat.java8.FutureConverters.CompletionStageOps import scala.concurrent.ExecutionContext.Implicits.global +import scala.concurrent.duration._ +import scala.concurrent.{Await, Future} trait StyxClientSupplier { val TWO_SECONDS: Int = 2 * 1000 val FIVE_SECONDS: Int = 5 * 1000 - val client: HttpClient = new StyxHttpClient.Builder() + val client: StyxHttpClient = new StyxHttpClient.Builder() .threadName("scalatest-e2e-client") .connectTimeout(1000) .maxHeaderSize(2 * 8192) .build() - val httpsClient: HttpClient = new StyxHttpClient.Builder() - .threadName("scalatest-e2e-secure-client") - .connectTimeout(1000) - .maxHeaderSize(2 * 8192) - .tlsSettings(new TlsSettings.Builder().build()) - .build() - - private def doHttpRequest(request: FullHttpRequest, debug: Boolean = false): Future[FullHttpResponse] = client.sendRequest(request).toScala - private def doSecureRequest(request: FullHttpRequest): Future[FullHttpResponse] = httpsClient.sendRequest(request).toScala + private def doSecureRequest(request: FullHttpRequest): Future[FullHttpResponse] = client.secure().sendRequest(request).toScala private def doRequest(request: FullHttpRequest, debug: Boolean = false, secure: Boolean = false): Future[FullHttpResponse] = if (secure) doSecureRequest(request) From 6e2209d6c702a4fd16e385aff8a7ed2f8351c71c Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Fri, 14 Sep 2018 19:12:30 +0100 Subject: [PATCH 04/15] End to end tests: - Reduce number threads used. - Eliminate thread leaks. --- .../src/test/java/com/hotels/styx/metrics/StyxMetrics.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system-tests/e2e-suite/src/test/java/com/hotels/styx/metrics/StyxMetrics.java b/system-tests/e2e-suite/src/test/java/com/hotels/styx/metrics/StyxMetrics.java index d2fe81fa62..b6c06410ba 100644 --- a/system-tests/e2e-suite/src/test/java/com/hotels/styx/metrics/StyxMetrics.java +++ b/system-tests/e2e-suite/src/test/java/com/hotels/styx/metrics/StyxMetrics.java @@ -19,7 +19,6 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Joiner; -import com.hotels.styx.client.HttpClient; import com.hotels.styx.api.FullHttpResponse; import com.hotels.styx.client.StyxHttpClient; import com.hotels.styx.support.Meter; @@ -159,8 +158,9 @@ private static Object toGauge(Map map) { } private static String downloadJsonString(String host, int port) { - HttpClient client = new StyxHttpClient.Builder().build(); + StyxHttpClient client = new StyxHttpClient.Builder().build(); FullHttpResponse response = await(client.sendRequest(get(format("http://%s:%d/admin/metrics", host, port)).build())); + await(client.shutdown()); return response.bodyAs(UTF_8); } From f9d5621cecd5dbb3543ab325b61916fc2f2f7ed7 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Fri, 14 Sep 2018 20:23:20 +0100 Subject: [PATCH 05/15] Reduce thread pressure. --- .../com/hotels/styx/StyxClientSupplier.scala | 20 +++++++++++-------- .../plugins/DoubleSubscribingPluginSpec.scala | 1 - 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala index 97c9262cb4..e728087e42 100644 --- a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala +++ b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala @@ -17,13 +17,16 @@ package com.hotels.styx import com.hotels.styx.api._ import com.hotels.styx.client.StyxHttpClient +import org.scalatest.{BeforeAndAfterAll, Suite} import scala.compat.java8.FutureConverters.CompletionStageOps import scala.concurrent.ExecutionContext.Implicits.global import scala.concurrent.duration._ import scala.concurrent.{Await, Future} -trait StyxClientSupplier { +trait StyxClientSupplier extends BeforeAndAfterAll { + this: Suite => + val TWO_SECONDS: Int = 2 * 1000 val FIVE_SECONDS: Int = 5 * 1000 @@ -33,21 +36,22 @@ trait StyxClientSupplier { .maxHeaderSize(2 * 8192) .build() - private def doHttpRequest(request: FullHttpRequest, debug: Boolean = false): Future[FullHttpResponse] = client.sendRequest(request).toScala - - private def doSecureRequest(request: FullHttpRequest): Future[FullHttpResponse] = client.secure().sendRequest(request).toScala + override protected def afterAll() = { + client.shutdown() + super.afterAll() + } - private def doRequest(request: FullHttpRequest, debug: Boolean = false, secure: Boolean = false): Future[FullHttpResponse] = if (secure) - doSecureRequest(request) + private def doRequest(request: FullHttpRequest, secure: Boolean = false): Future[FullHttpResponse] = if (secure) + client.secure().sendRequest(request).toScala else - doHttpRequest(request, debug = debug) + client.sendRequest(request).toScala def decodedRequest(request: FullHttpRequest, debug: Boolean = false, maxSize: Int = 1024 * 1024, timeout: Duration = 30.seconds, secure: Boolean = false ): FullHttpResponse = { - val future = doRequest(request, debug = debug, secure = secure) + val future = doRequest(request, secure = secure) .map(response => { if (debug) { println("StyxClientSupplier: received response for: " + request.url().path()) diff --git a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/plugins/DoubleSubscribingPluginSpec.scala b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/plugins/DoubleSubscribingPluginSpec.scala index efdf9d3859..15859b7f51 100644 --- a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/plugins/DoubleSubscribingPluginSpec.scala +++ b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/plugins/DoubleSubscribingPluginSpec.scala @@ -18,7 +18,6 @@ package com.hotels.styx.plugins import ch.qos.logback.classic.Level.ERROR import com.hotels.styx.MockServer.responseSupplier import com.hotels.styx.api.HttpResponseStatus._ -import com.hotels.styx.api.plugins.spi.PluginException import com.hotels.styx.api.{HttpResponse, FullHttpResponse => StyxFullHttpResponse} import com.hotels.styx.proxy.HttpErrorStatusCauseLogger import com.hotels.styx.support.configuration.{HttpBackend, Origins, ProxyConfig, StyxConfig} From cc28e130e95f17240379bc08f2a0dcb312e5ad7e Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Mon, 17 Sep 2018 17:38:57 +0100 Subject: [PATCH 06/15] Full test coverage. Refactor StyxHttpClient. --- .../hotels/styx/client/StyxHttpClient.java | 64 ++-- .../styx/client/StyxHttpClientTest.java | 303 +++++++----------- 2 files changed, 156 insertions(+), 211 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 3457c6a6b0..f637e08033 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -15,6 +15,7 @@ */ package com.hotels.styx.client; +import com.google.common.annotations.VisibleForTesting; import com.google.common.net.HostAndPort; import com.hotels.styx.api.FullHttpRequest; import com.hotels.styx.api.FullHttpResponse; @@ -37,6 +38,7 @@ import static com.hotels.styx.api.extension.Origin.newOriginBuilder; import static com.hotels.styx.client.HttpConfig.newHttpConfigBuilder; import static com.hotels.styx.common.CompletableFutures.fromSingleObservable; +import static java.util.Objects.requireNonNull; /** * A client that uses netty as transport. @@ -45,11 +47,11 @@ public final class StyxHttpClient implements HttpClient { private static final int DEFAULT_HTTPS_PORT = 443; private static final int DEFAULT_HTTP_PORT = 80; - private final ClientParameters clientParameters; + private final TransactionParameters transactionParameters; final NettyConnectionFactory connectionFactory; private StyxHttpClient(Builder builder) { - clientParameters = new ClientParameters(builder); + transactionParameters = new TransactionParameters(builder); connectionFactory = builder.connectionFactory; } @@ -58,36 +60,25 @@ public CompletableFuture shutdown() { } public HttpClient.Transaction secure() { - ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().secure(true)); + TransactionParameters parameters = new TransactionParameters(this.transactionParameters.newBuilder().secure(true)); return new StyxHttpClientTransaction(connectionFactory, parameters); } public HttpClient.Transaction secure(boolean secure) { - if (!secure && this.clientParameters.https()) { - throw new IllegalArgumentException("An insecure HTTP transaction is not allowed with HTTPS configuration."); - } - ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().secure(secure)); - return new StyxHttpClientTransaction(connectionFactory, parameters); - } - - public HttpClient.Transaction userAgent(String userAgent) { - ClientParameters parameters = new ClientParameters(this.clientParameters.newBuilder().userAgent(userAgent)); + TransactionParameters parameters = new TransactionParameters(this.transactionParameters.newBuilder().secure(secure)); return new StyxHttpClientTransaction(connectionFactory, parameters); } public CompletableFuture sendRequest(FullHttpRequest request) { - return sendRequestInternal(connectionFactory, request, this.clientParameters); + return sendRequestInternal(connectionFactory, request, this.transactionParameters); } - private static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, ClientParameters parent) { + @VisibleForTesting + static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, TransactionParameters parent) { FullHttpRequest networkRequest = parent.addUserAgent(request); Origin origin = originFromRequest(networkRequest, parent.https()); - TlsSettings tlsSettings = parent.tlsSettings().orElseGet(() -> parent.https() ? new TlsSettings.Builder().build() : null); - - SslContext sslContext = Optional.ofNullable(tlsSettings) - .map(SslContextFactory::get) - .orElse(null); + SslContext sslContext = getSslContext(parent.isHttps, parent.tlsSettings); Observable responseObservable = connectionFactory.createConnection(origin, parent.connectionSettings(), sslContext) .flatMap(connection -> @@ -100,6 +91,17 @@ private static CompletableFuture sendRequestInternal(NettyConn } + private static SslContext getSslContext(boolean isHttps, TlsSettings tlsSettings) { + if (isHttps) { + return Optional.of(tlsSettings != null ? tlsSettings : TransactionParameters.DEFAULT_TLS_SETTINGS) + .map(SslContextFactory::get) + .orElse(null); + } else { + return null; + } + + } + private static Origin originFromRequest(FullHttpRequest request, Boolean isHttps) { String hostAndPort = request.header(HOST) .orElseGet(() -> { @@ -118,14 +120,15 @@ private static Origin originFromRequest(FullHttpRequest request, Boolean isHttps } - static class ClientParameters { + static class TransactionParameters { + static final TlsSettings DEFAULT_TLS_SETTINGS = new TlsSettings.Builder().build(); private final ConnectionSettings connectionSettings; private final Optional userAgent; private final int maxResponseSize; private final boolean isHttps; private TlsSettings tlsSettings; - private ClientParameters(Builder builder) { + TransactionParameters(Builder builder) { this.userAgent = Optional.ofNullable(builder.userAgent); this.connectionSettings = builder.connectionSettings; this.maxResponseSize = builder.maxResponseSize; @@ -154,11 +157,10 @@ public String threadName() { } private FullHttpRequest addUserAgent(FullHttpRequest request) { - return Optional.of(request) - .filter(req -> !req.header(USER_AGENT).isPresent()) - .flatMap(req -> userAgent.map(userAgent -> request.newBuilder() - .addHeader(USER_AGENT, userAgent) - .build())) + return userAgent.map(value -> + request.newBuilder() + .header(USER_AGENT, value) + .build()) .orElse(request); } @@ -166,7 +168,7 @@ public Builder newBuilder() { Builder builder = new Builder() .connectTimeout(connectionSettings.connectTimeoutMillis()) .maxHeaderSize(maxResponseSize) - .tlsSettings(tlsSettings) + .tlsSettings(Optional.ofNullable(tlsSettings).orElse(DEFAULT_TLS_SETTINGS)) .secure(isHttps); userAgent.ifPresent(builder::userAgent); @@ -191,7 +193,7 @@ static class StyxHttpClientTransaction implements HttpClient.Transaction { private Builder builder; private NettyConnectionFactory connectionFactory; - public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, ClientParameters parent) { + public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, TransactionParameters parent) { this.builder = parent.newBuilder(); this.connectionFactory = connectionFactory; } @@ -215,7 +217,7 @@ public StreamingTransaction streaming() { @Override public CompletableFuture sendRequest(FullHttpRequest request) { - return StyxHttpClient.sendRequestInternal(connectionFactory, request, new ClientParameters(builder)); + return StyxHttpClient.sendRequestInternal(connectionFactory, request, new TransactionParameters(builder)); } } @@ -260,8 +262,8 @@ public Builder maxResponseSize(int maxResponseSize) { } public Builder tlsSettings(TlsSettings tlsSettings) { - this.tlsSettings = tlsSettings; - this.isHttps = tlsSettings != null; + this.tlsSettings = requireNonNull(tlsSettings); + this.isHttps = true; return this; } diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index edb8f4f078..05fb1cdacd 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -19,17 +19,17 @@ import com.github.tomakehurst.wiremock.client.WireMock; import com.hotels.styx.api.FullHttpRequest; import com.hotels.styx.api.FullHttpResponse; -import com.hotels.styx.api.HttpRequest; -import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.exceptions.OriginUnreachableException; import com.hotels.styx.api.extension.Origin; import com.hotels.styx.api.extension.service.TlsSettings; +import com.hotels.styx.client.netty.connectionpool.NettyConnectionFactory; +import io.netty.handler.ssl.SslContext; +import org.mockito.ArgumentCaptor; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import rx.Observable; -import java.io.IOException; import java.util.concurrent.ExecutionException; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; @@ -39,18 +39,21 @@ import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; import static com.hotels.styx.api.FullHttpRequest.get; import static com.hotels.styx.api.HttpHeaderNames.HOST; +import static com.hotels.styx.api.HttpHeaderNames.USER_AGENT; import static com.hotels.styx.api.HttpResponseStatus.OK; import static com.hotels.styx.common.StyxFutures.await; import static com.hotels.styx.support.server.UrlMatchingStrategies.urlStartingWith; +import static java.lang.String.format; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; public class StyxHttpClientTest { - private FullHttpRequest anyRequest; - private static int MAX_LENGTH = 1024; + private FullHttpRequest httpRequest; + private FullHttpRequest secureRequest; private WireMockServer server; @BeforeMethod @@ -59,8 +62,12 @@ public void setUp() { server.start(); server.stubFor(WireMock.get(urlStartingWith("/")).willReturn(aResponse().withStatus(200))); - anyRequest = get("/foo.txt") - .header(HOST, "localhost:1234") + httpRequest = get("/") + .header(HOST, hostString(server.port())) + .build(); + + secureRequest = get("/") + .header(HOST, hostString(server.httpsPort())) .build(); } @@ -71,13 +78,13 @@ public void tearDown() { } @Test - public void closesThreadpoolAfterUse() throws InterruptedException, ExecutionException { + public void closesThreadPoolAfterUse() throws InterruptedException, ExecutionException { StyxHttpClient client = new StyxHttpClient.Builder() .threadName("test-client") .build(); - // Ensures that thread is created - client.sendRequest(httpRequest(server.port())).get(); + // Ensures that a thread is created before the assertions + client.sendRequest(httpRequest).get(); assertThat(threadExists("test-client"), is(true)); @@ -100,7 +107,7 @@ public void usesDefaultUserAgent() throws ExecutionException, InterruptedExcepti .build(); FullHttpResponse response = client - .sendRequest(httpRequest(server.port())) + .sendRequest(httpRequest) .get(); assertThat(response.status(), is(OK)); @@ -111,23 +118,38 @@ public void usesDefaultUserAgent() throws ExecutionException, InterruptedExcepti } @Test - public void usesPerTransactionUserAgent() throws ExecutionException, InterruptedException { + public void doesNotSetAnyUserAgentIfNotSpecified() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() - .userAgent("Simple-Client-Parent-Settings") .build(); - FullHttpResponse response = client - .userAgent("Simple-Client-Transaction-Settings") - .sendRequest(httpRequest(server.port())) - .get(); + client.sendRequest(httpRequest).get(); + + server.verify( + getRequestedFor(urlEqualTo("/")) + .withoutHeader("User-Agent") + ); + } + + @Test + public void replacesUserAgentIfAlreadyPresentInRequest() throws ExecutionException, InterruptedException { + StyxHttpClient client = new StyxHttpClient.Builder() + .userAgent("My default user agent value") + .build(); + + FullHttpRequest request = get("/") + .header(HOST, hostString(server.port())) + .header(USER_AGENT, "My previous user agent") + .build(); + + client.sendRequest(request).get(); - assertThat(response.status(), is(OK)); server.verify( getRequestedFor(urlEqualTo("/")) - .withHeader("User-Agent", equalTo("Simple-Client-Transaction-Settings")) + .withHeader("User-Agent", equalTo("My default user agent value")) ); } + @Test public void usesDefaultTlsSettings() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() @@ -135,7 +157,7 @@ public void usesDefaultTlsSettings() throws ExecutionException, InterruptedExcep .build(); FullHttpResponse response = client - .sendRequest(httpRequest(server.httpsPort())) + .sendRequest(secureRequest) .get(); assertThat(response.status(), is(OK)); @@ -148,7 +170,7 @@ public void overridesTlsSettingsWithSecure() throws ExecutionException, Interrup FullHttpResponse response = client .secure() - .sendRequest(httpRequest(server.httpsPort())) + .sendRequest(secureRequest) .get(); assertThat(response.status(), is(OK)); @@ -161,121 +183,86 @@ public void overridesTlsSettingsWithSecureBoolean() throws ExecutionException, I FullHttpResponse response = client .secure(true) - .sendRequest(httpRequest(server.httpsPort())) + .sendRequest(secureRequest) .get(); assertThat(response.status(), is(OK)); } - @Test(expectedExceptions = IllegalArgumentException.class, - expectedExceptionsMessageRegExp = "An insecure HTTP transaction is not allowed with HTTPS configuration.") - public void canNotDowngradeDefaultSecureSettings() { - StyxHttpClient client = new StyxHttpClient.Builder() - .tlsSettings(new TlsSettings.Builder().build()) - .build(); - - client.secure(false); - } - @Test - public void disablesTlsIfNullArgumentIsPassed() throws ExecutionException, InterruptedException { + public void overridesTlsSettingsWithSecureBooleanFalse() throws ExecutionException, InterruptedException { StyxHttpClient client = new StyxHttpClient.Builder() - .tlsSettings(null) + .tlsSettings(new TlsSettings.Builder().build()) .build(); FullHttpResponse response = client .secure(false) - .sendRequest(httpRequest(server.port())) + .sendRequest(httpRequest) .get(); assertThat(response.status(), is(OK)); } - @Test - public void sendsHttp() { - FullHttpResponse response = await(httpClient().sendRequest(httpRequest(server.port()))); - assertThat(response.status(), is(OK)); + @Test(expectedExceptions = NullPointerException.class) + public void requiresValidTlsSettins() { + new StyxHttpClient.Builder() + .tlsSettings(null) + .build(); } - @Test - public void sendsHttps() { - FullHttpResponse response = await(httpsClient().sendRequest(httpsRequest(server.httpsPort()))); - assertThat(response.status(), is(OK)); + // TODO: Mikko: + // Note: this test case might fail depending on your ISP's configuration. + // Some ISPs may redirect failed DNS lookups to a specific landing page for + // advertising purposes. + @Test(expectedExceptions = OriginUnreachableException.class) + public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Throwable { + try { + new StyxHttpClient.Builder() + .build() + .sendRequest(get("/foo.txt").header(HOST, "a.b.c").build()).get(); + } catch (ExecutionException cause) { + throw cause.getCause(); + } } -// @Test(expectedExceptions = OriginUnreachableException.class) -// public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Throwable { -// try { -// httpClient().sendRequest(get("/foo.txt").header(HOST, "a.b.c").build()).get(); -// } catch (ExecutionException cause) { -// throw cause.getCause(); -// } -// } - - @Test(expectedExceptions = Exception.class) - public void cannotSendHttpsWhenConfiguredForHttp() { - FullHttpResponse response = await(httpClient().sendRequest(httpsRequest(server.httpsPort()))); + @Test + public void sendsMessagesInOriginUrlFormat() throws ExecutionException, InterruptedException { + FullHttpResponse response = new StyxHttpClient.Builder() + .build() + .sendRequest(get("/index.html").header(HOST, hostString(server.port())).build()) + .get(); + assertThat(response.status(), is(OK)); + server.verify( + getRequestedFor(urlEqualTo("/index.html")) + .withHeader("Host", equalTo(hostString(server.port()))) + ); } - @Test(expectedExceptions = Exception.class) - public void cannotSendHttpWhenConfiguredForHttps() throws IOException { - FullHttpResponse response = await(httpsClient().sendRequest(httpRequest(server.port()))); + + @Test(enabled = false) + /* + * Wiremock (or Jetty server) origin converts an absolute URL to an origin + * form. Therefore we are unable to use an origin to verify that client used + * an absolute URL. However I (Mikko) have verified with WireShark that the + * request is indeed sent in absolute form. + */ + public void sendsMessagesInAbsoluteUrlFormat() throws ExecutionException, InterruptedException { + FullHttpResponse response = new StyxHttpClient.Builder() + .build() + .sendRequest(get(format("http://%s/index.html", hostString(server.port()))).build()) + .get(); + assertThat(response.status(), is(OK)); + server.verify( + getRequestedFor(urlEqualTo(format("http://%s/index.html", hostString(server.port())))) + .withHeader("Host", equalTo(hostString(server.port()))) + ); } -// @Test -// public void willNotSetAnyUserAgentIfNotSpecified() { -// Connection mockConnection = mock(Connection.class); -// when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); -// -// StyxHttpClient client = new StyxHttpClient.Builder() -// .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) -// .build(); -// -// client.sendRequest(anyRequest); -// -// ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); -// verify(mockConnection).write(captor.capture()); -// assertThat(captor.getValue().header(USER_AGENT), isAbsent()); -// } -// -// @Test -// public void setsTheSpecifiedUserAgent() { -// Connection mockConnection = mock(Connection.class); -// when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); -// -// StyxHttpClient client = new StyxHttpClient.Builder() -// .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) -// .userAgent("Styx/5.6") -// .build(); -// -// client.sendRequest(anyRequest); -// -// ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); -// verify(mockConnection).write(captor.capture()); -// assertThat(captor.getValue().header(USER_AGENT), isValue("Styx/5.6")); -// } -// -// @Test -// public void retainsTheUserAgentStringFromTheRequest() { -// Connection mockConnection = mock(Connection.class); -// when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); -// -// StyxHttpClient client = new StyxHttpClient.Builder() -// .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) -// .userAgent("Styx/5.6") -// .build(); -// -// client.sendRequest(get("/foo.txt") -// .header(USER_AGENT, "Foo/Bar") -// .header(HOST, "localhost:1234") -// .build()); -// -// ArgumentCaptor captor = ArgumentCaptor.forClass(HttpRequest.class); -// verify(mockConnection).write(captor.capture()); -// assertThat(captor.getValue().header(USER_AGENT), isValue("Foo/Bar")); -// } + private String hostString(int port) { + return "localhost:" + port; + } @Test(expectedExceptions = IllegalArgumentException.class) public void requestWithNoHostOrUrlAuthorityCausesException() { @@ -286,82 +273,38 @@ public void requestWithNoHostOrUrlAuthorityCausesException() { await(client.sendRequest(request)); } -// @Test -// public void sendsToDefaultHttpPort() { -// Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); -// -// StyxHttpClient client = new StyxHttpClient.Builder() -// .setConnectionFactory(connectionFactory) -// .build(); -// -// client.sendRequest(get("/") -// .header(HOST, "localhost") -// .build()); -// -// ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); -// verify(connectionFactory).createConnection(originCaptor.capture(), any(ConnectionSettings.class)); -// -// assertThat(originCaptor.getValue().port(), is(80)); -// } -// -// @Test -// public void sendsToDefaultHttpsPort() { -// Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); -// TlsSettings tlsSettings = mock(TlsSettings.class); -// -// StyxHttpClient client = new StyxHttpClient.Builder() -// .setConnectionFactory(connectionFactory) -// .tlsSettings(tlsSettings) -// .build(); -// -// client.sendRequest(get("/") -// .header(HOST, "localhost") -// .build()); -// -// ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); -// verify(connectionFactory).createConnection(originCaptor.capture(), any(ConnectionSettings.class)); -// -// assertThat(originCaptor.getValue().port(), is(443)); -// } - - private StyxHttpClient httpClient() { - return new StyxHttpClient.Builder() - .build(); - } + @Test + public void sendsToDefaultHttpPort() { + NettyConnectionFactory factory = mockConnectionFactory(); + ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); - private StyxHttpClient httpsClient() { - return new StyxHttpClient.Builder() - .connectTimeout(1000) - .tlsSettings(new TlsSettings.Builder() - .authenticate(false) - .build()) - .responseTimeout(6000) - .build(); - } + StyxHttpClient.sendRequestInternal(factory, get("/") + .header(HOST, "localhost") + .build(), + new StyxHttpClient.TransactionParameters(new StyxHttpClient.Builder())); - private FullHttpRequest httpRequest(int port) { - return get("http://localhost:" + port) - .header(HOST, "localhost:" + port) - .build(); + verify(factory).createConnection(originCaptor.capture(), any(ConnectionSettings.class), any(SslContext.class)); + assertThat(originCaptor.getValue().port(), is(80)); } - private FullHttpRequest httpsRequest(int port) { - return get("https://localhost:" + port) - .header(HOST, "localhost:" + port) - .build(); - } + @Test + public void sendsToDefaultHttpsPort() { + NettyConnectionFactory factory = mockConnectionFactory(); + ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); + + StyxHttpClient.sendRequestInternal(factory, get("/") + .header(HOST, "localhost") + .build(), + new StyxHttpClient.TransactionParameters(new StyxHttpClient.Builder().secure(true))); - private Connection mockConnection(HttpResponse response) { - Connection connection = mock(Connection.class); - when(connection.write(any(HttpRequest.class))).thenReturn(Observable.just(response)); - return connection; + verify(factory).createConnection(originCaptor.capture(), any(ConnectionSettings.class), any(SslContext.class)); + assertThat(originCaptor.getValue().port(), is(443)); } - private Connection.Factory mockConnectionFactory(Connection connection) { - Connection.Factory factory = mock(Connection.Factory.class); - when(factory.createConnection(any(Origin.class), any(ConnectionSettings.class))) - .thenReturn(Observable.just(connection)); + private static NettyConnectionFactory mockConnectionFactory() { + NettyConnectionFactory factory = mock(NettyConnectionFactory.class); + when(factory.createConnection(any(Origin.class), any(ConnectionSettings.class), any(SslContext.class))) + .thenReturn(Observable.just(mock(Connection.class))); return factory; } - -} +} \ No newline at end of file From 64da47dd4efc23ef4c2f0e479b60a0b99c4ccf9b Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 09:23:14 +0100 Subject: [PATCH 07/15] Update origin health check monitor. --- .../styx/proxy/BackendServicesRouter.java | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java b/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java index e8f1d1f30a..ad7cc97450 100644 --- a/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java +++ b/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java @@ -205,12 +205,7 @@ private static OriginHealthCheckFunction originHealthCheckFunction( ConnectionSettings connectionSettings = new ConnectionSettings( connectionPoolSettings.connectTimeoutMillis()); - StyxHttpClient client = new StyxHttpClient.Builder() - .connectTimeout(connectionSettings.connectTimeoutMillis()) - .threadName("Health-Check-Monitor-" + appId) - .userAgent("Styx/" + styxVersion) - .tlsSettings(tlsSettings.orElse(null)) - .build(); + StyxHttpClient client = healthCheckClient(appId, tlsSettings, styxVersion, connectionSettings); String healthCheckUri = healthCheckConfig .uri() @@ -219,6 +214,17 @@ private static OriginHealthCheckFunction originHealthCheckFunction( return new UrlRequestHealthCheck(healthCheckUri, client, metricRegistry); } + private static StyxHttpClient healthCheckClient(Id appId, Optional tlsSettings, String styxVersion, ConnectionSettings connectionSettings) { + StyxHttpClient.Builder builder = new StyxHttpClient.Builder() + .connectTimeout(connectionSettings.connectTimeoutMillis()) + .threadName("Health-Check-Monitor-" + appId) + .userAgent("Styx/" + styxVersion); + + tlsSettings.ifPresent(builder::tlsSettings); + + return builder.build(); + } + private static class ProxyToClientPipeline implements HttpHandler { private final HttpHandler client; private final OriginsInventory originsInventory; From 5b3e76cba1ee9c6793aa09682c5845cffdfd65b4 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 10:41:05 +0100 Subject: [PATCH 08/15] Refactor. --- .../hotels/styx/client/StyxHttpClient.java | 157 +++++------------- .../client/StyxHttpClientTransaction.java | 54 ++++++ .../styx/client/TransactionParameters.java | 96 +++++++++++ .../styx/client/StyxHttpClientTest.java | 28 +++- 4 files changed, 217 insertions(+), 118 deletions(-) create mode 100644 components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java create mode 100644 components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index f637e08033..139c32ef0c 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -33,7 +33,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.hotels.styx.api.HttpHeaderNames.HOST; -import static com.hotels.styx.api.HttpHeaderNames.USER_AGENT; import static com.hotels.styx.api.StyxInternalObservables.toRxObservable; import static com.hotels.styx.api.extension.Origin.newOriginBuilder; import static com.hotels.styx.client.HttpConfig.newHttpConfigBuilder; @@ -74,18 +73,21 @@ public CompletableFuture sendRequest(FullHttpRequest request) } @VisibleForTesting - static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, TransactionParameters parent) { - FullHttpRequest networkRequest = parent.addUserAgent(request); - Origin origin = originFromRequest(networkRequest, parent.https()); - - SslContext sslContext = getSslContext(parent.isHttps, parent.tlsSettings); - - Observable responseObservable = connectionFactory.createConnection(origin, parent.connectionSettings(), sslContext) - .flatMap(connection -> - connection.write(networkRequest.toStreamingRequest()) - .flatMap(response -> toRxObservable(response.toFullResponse(parent.maxResponseSize()))) - .doOnTerminate(connection::close) - ); + static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, TransactionParameters params) { + FullHttpRequest networkRequest = params.addUserAgent(request); + Origin origin = originFromRequest(networkRequest, params.https()); + + SslContext sslContext = getSslContext(params.https(), params.tlsSettings().orElse(null)); + + Observable responseObservable = connectionFactory.createConnection( + origin, + new ConnectionSettings(params.connectionSettings()), + sslContext + ).flatMap(connection -> + connection.write(networkRequest.toStreamingRequest()) + .flatMap(response -> toRxObservable(response.toFullResponse(params.maxResponseSize()))) + .doOnTerminate(connection::close) + ); return fromSingleObservable(responseObservable); @@ -120,107 +122,6 @@ private static Origin originFromRequest(FullHttpRequest request, Boolean isHttps } - static class TransactionParameters { - static final TlsSettings DEFAULT_TLS_SETTINGS = new TlsSettings.Builder().build(); - private final ConnectionSettings connectionSettings; - private final Optional userAgent; - private final int maxResponseSize; - private final boolean isHttps; - private TlsSettings tlsSettings; - - TransactionParameters(Builder builder) { - this.userAgent = Optional.ofNullable(builder.userAgent); - this.connectionSettings = builder.connectionSettings; - this.maxResponseSize = builder.maxResponseSize; - this.isHttps = builder.isHttps; - this.tlsSettings = builder.tlsSettings; - } - - private Optional userAgent() { - return userAgent; - } - - private ConnectionSettings connectionSettings() { - return connectionSettings; - } - - private int maxResponseSize() { - return maxResponseSize; - } - - private boolean https() { - return isHttps; - } - - public String threadName() { - return "SimpleHttpClientThread"; - } - - private FullHttpRequest addUserAgent(FullHttpRequest request) { - return userAgent.map(value -> - request.newBuilder() - .header(USER_AGENT, value) - .build()) - .orElse(request); - } - - public Builder newBuilder() { - Builder builder = new Builder() - .connectTimeout(connectionSettings.connectTimeoutMillis()) - .maxHeaderSize(maxResponseSize) - .tlsSettings(Optional.ofNullable(tlsSettings).orElse(DEFAULT_TLS_SETTINGS)) - .secure(isHttps); - - userAgent.ifPresent(builder::userAgent); - - return builder; - } - - public int maxHeaderSize() { - return 8192; - } - - public Optional tlsSettings() { - return Optional.ofNullable(tlsSettings); - } - - public int responseTimeout() { - return 60000; - } - } - - static class StyxHttpClientTransaction implements HttpClient.Transaction { - private Builder builder; - private NettyConnectionFactory connectionFactory; - - public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, TransactionParameters parent) { - this.builder = parent.newBuilder(); - this.connectionFactory = connectionFactory; - } - - @Override - public Transaction secure() { - this.builder.secure(true); - return this; - } - - @Override - public Transaction secure(boolean secure) { - this.builder.secure(secure); - return this; - } - - @Override - public StreamingTransaction streaming() { - return null; - } - - @Override - public CompletableFuture sendRequest(FullHttpRequest request) { - return StyxHttpClient.sendRequestInternal(connectionFactory, request, new TransactionParameters(builder)); - } - } - /** * Builder for {@link StyxHttpClient}. */ @@ -228,7 +129,7 @@ public static class Builder { private NettyConnectionFactory connectionFactory; private TlsSettings tlsSettings; private String userAgent; - private ConnectionSettings connectionSettings = new ConnectionSettings(1000); + private int connectTimeoutMillis = 1000; private int responseTimeout = 60000; private int maxResponseSize = 1024 * 100; private int maxHeaderSize = 8192; @@ -236,15 +137,23 @@ public static class Builder { private boolean isHttps; public Builder connectTimeout(int timeoutMs) { - this.connectionSettings = new ConnectionSettings(timeoutMs); + this.connectTimeoutMillis = timeoutMs; return this; } + int connectTimeoutMillis() { + return connectTimeoutMillis; + } + public Builder responseTimeout(int responseTimeoutMs) { this.responseTimeout = responseTimeoutMs; return this; } + int responseTimeout() { + return responseTimeout; + } + /** * Sets the user-agent header value to be included in requests. * @@ -256,22 +165,38 @@ public Builder userAgent(String userAgent) { return this; } + String userAgent() { + return this.userAgent; + } + public Builder maxResponseSize(int maxResponseSize) { this.maxResponseSize = maxResponseSize; return this; } + int maxResponseSize() { + return this.maxResponseSize; + } + public Builder tlsSettings(TlsSettings tlsSettings) { this.tlsSettings = requireNonNull(tlsSettings); this.isHttps = true; return this; } + TlsSettings tlsSettings() { + return this.tlsSettings; + } + public Builder secure(boolean secure) { this.isHttps = secure; return this; } + boolean https() { + return this.isHttps; + } + public Builder maxHeaderSize(int maxHeaderSize) { this.maxHeaderSize = maxHeaderSize; return this; diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java new file mode 100644 index 0000000000..c066625f20 --- /dev/null +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java @@ -0,0 +1,54 @@ +/* + Copyright (C) 2013-2018 Expedia 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 com.hotels.styx.client; + +import com.hotels.styx.api.FullHttpRequest; +import com.hotels.styx.api.FullHttpResponse; +import com.hotels.styx.client.netty.connectionpool.NettyConnectionFactory; + +import java.util.concurrent.CompletableFuture; + +class StyxHttpClientTransaction implements HttpClient.Transaction { + private StyxHttpClient.Builder builder; + private NettyConnectionFactory connectionFactory; + + public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, TransactionParameters parent) { + this.builder = parent.newBuilder(); + this.connectionFactory = connectionFactory; + } + + @Override + public HttpClient.Transaction secure() { + this.builder.secure(true); + return this; + } + + @Override + public HttpClient.Transaction secure(boolean secure) { + this.builder.secure(secure); + return this; + } + + @Override + public HttpClient.StreamingTransaction streaming() { + return null; + } + + @Override + public CompletableFuture sendRequest(FullHttpRequest request) { + return StyxHttpClient.sendRequestInternal(connectionFactory, request, new TransactionParameters(builder)); + } +} diff --git a/components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java b/components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java new file mode 100644 index 0000000000..ca69e2680c --- /dev/null +++ b/components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java @@ -0,0 +1,96 @@ +/* + Copyright (C) 2013-2018 Expedia 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 com.hotels.styx.client; + +import com.hotels.styx.api.FullHttpRequest; +import com.hotels.styx.api.extension.service.TlsSettings; + +import java.util.Optional; + +import static com.hotels.styx.api.HttpHeaderNames.USER_AGENT; + +class TransactionParameters { + static final TlsSettings DEFAULT_TLS_SETTINGS = new TlsSettings.Builder().build(); + private final int connectTimeoutMillis; + private final Optional userAgent; + private final int maxResponseSize; + private final boolean isHttps; + private final int responseTimeout; + private TlsSettings tlsSettings; + + TransactionParameters(StyxHttpClient.Builder builder) { + this.userAgent = Optional.ofNullable(builder.userAgent()); + this.connectTimeoutMillis = builder.connectTimeoutMillis(); + this.maxResponseSize = builder.maxResponseSize(); + this.isHttps = builder.https(); + this.tlsSettings = builder.tlsSettings(); + this.responseTimeout = builder.responseTimeout(); + } + + Optional userAgent() { + return userAgent; + } + + int connectionSettings() { + return connectTimeoutMillis; + } + + int maxResponseSize() { + return maxResponseSize; + } + + boolean https() { + return isHttps; + } + + String threadName() { + return "SimpleHttpClientThread"; + } + + int responseTimeout() { + return this.responseTimeout; + } + + FullHttpRequest addUserAgent(FullHttpRequest request) { + return userAgent.map(value -> + request.newBuilder() + .header(USER_AGENT, value) + .build()) + .orElse(request); + } + + StyxHttpClient.Builder newBuilder() { + StyxHttpClient.Builder builder = new StyxHttpClient.Builder() + .connectTimeout(connectTimeoutMillis) + .maxHeaderSize(maxResponseSize) + .tlsSettings(Optional.ofNullable(tlsSettings).orElse(DEFAULT_TLS_SETTINGS)) + .responseTimeout(responseTimeout) + .secure(isHttps); + + userAgent.ifPresent(builder::userAgent); + + return builder; + } + + int maxHeaderSize() { + return 8192; + } + + Optional tlsSettings() { + return Optional.ofNullable(tlsSettings); + } + +} diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index 05fb1cdacd..c77b57c85b 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -20,6 +20,7 @@ import com.hotels.styx.api.FullHttpRequest; import com.hotels.styx.api.FullHttpResponse; import com.hotels.styx.api.exceptions.OriginUnreachableException; +import com.hotels.styx.api.exceptions.ResponseTimeoutException; import com.hotels.styx.api.extension.Origin; import com.hotels.styx.api.extension.service.TlsSettings; import com.hotels.styx.client.netty.connectionpool.NettyConnectionFactory; @@ -44,6 +45,7 @@ import static com.hotels.styx.common.StyxFutures.await; import static com.hotels.styx.support.server.UrlMatchingStrategies.urlStartingWith; import static java.lang.String.format; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.mockito.Matchers.any; @@ -239,6 +241,28 @@ public void sendsMessagesInOriginUrlFormat() throws ExecutionException, Interrup ); } + @Test(expectedExceptions = ResponseTimeoutException.class) + public void defaultResponseTimeout() throws Throwable { + StyxHttpClient client = new StyxHttpClient.Builder() + .responseTimeout(1000) + .build(); + + server.stubFor(WireMock.get(urlStartingWith("/slowResponse")) + .willReturn(aResponse() + .withStatus(200) + .withFixedDelay(3000) + )); + + try { + client.sendRequest( + get("/slowResponse") + .header(HOST, hostString(server.port())) + .build()) + .get(2, SECONDS); + } catch (ExecutionException e) { + throw e.getCause(); + } + } @Test(enabled = false) /* @@ -281,7 +305,7 @@ public void sendsToDefaultHttpPort() { StyxHttpClient.sendRequestInternal(factory, get("/") .header(HOST, "localhost") .build(), - new StyxHttpClient.TransactionParameters(new StyxHttpClient.Builder())); + new TransactionParameters(new StyxHttpClient.Builder())); verify(factory).createConnection(originCaptor.capture(), any(ConnectionSettings.class), any(SslContext.class)); assertThat(originCaptor.getValue().port(), is(80)); @@ -295,7 +319,7 @@ public void sendsToDefaultHttpsPort() { StyxHttpClient.sendRequestInternal(factory, get("/") .header(HOST, "localhost") .build(), - new StyxHttpClient.TransactionParameters(new StyxHttpClient.Builder().secure(true))); + new TransactionParameters(new StyxHttpClient.Builder().secure(true))); verify(factory).createConnection(originCaptor.capture(), any(ConnectionSettings.class), any(SslContext.class)); assertThat(originCaptor.getValue().port(), is(443)); From 6ed6c36e516abbac6841186055a15324facf836c Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 11:06:14 +0100 Subject: [PATCH 09/15] Eliminate TransactionParameters class. --- .../hotels/styx/client/StyxHttpClient.java | 71 ++++++++++---- .../client/StyxHttpClientTransaction.java | 6 +- .../styx/client/TransactionParameters.java | 96 ------------------- .../styx/client/StyxHttpClientTest.java | 4 +- 4 files changed, 60 insertions(+), 117 deletions(-) delete mode 100644 components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 139c32ef0c..93a24f60f7 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -33,6 +33,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.hotels.styx.api.HttpHeaderNames.HOST; +import static com.hotels.styx.api.HttpHeaderNames.USER_AGENT; import static com.hotels.styx.api.StyxInternalObservables.toRxObservable; import static com.hotels.styx.api.extension.Origin.newOriginBuilder; import static com.hotels.styx.client.HttpConfig.newHttpConfigBuilder; @@ -46,12 +47,12 @@ public final class StyxHttpClient implements HttpClient { private static final int DEFAULT_HTTPS_PORT = 443; private static final int DEFAULT_HTTP_PORT = 80; - private final TransactionParameters transactionParameters; - final NettyConnectionFactory connectionFactory; + private final Builder transactionParameters; + private final NettyConnectionFactory connectionFactory; - private StyxHttpClient(Builder builder) { - transactionParameters = new TransactionParameters(builder); - connectionFactory = builder.connectionFactory; + private StyxHttpClient(NettyConnectionFactory connectionFactory, Builder parameters) { + transactionParameters = parameters; + this.connectionFactory = connectionFactory; } public CompletableFuture shutdown() { @@ -59,13 +60,11 @@ public CompletableFuture shutdown() { } public HttpClient.Transaction secure() { - TransactionParameters parameters = new TransactionParameters(this.transactionParameters.newBuilder().secure(true)); - return new StyxHttpClientTransaction(connectionFactory, parameters); + return new StyxHttpClientTransaction(connectionFactory, this.transactionParameters.copy().secure(true)); } public HttpClient.Transaction secure(boolean secure) { - TransactionParameters parameters = new TransactionParameters(this.transactionParameters.newBuilder().secure(secure)); - return new StyxHttpClientTransaction(connectionFactory, parameters); + return new StyxHttpClientTransaction(connectionFactory, this.transactionParameters.copy().secure(secure)); } public CompletableFuture sendRequest(FullHttpRequest request) { @@ -73,15 +72,15 @@ public CompletableFuture sendRequest(FullHttpRequest request) } @VisibleForTesting - static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, TransactionParameters params) { - FullHttpRequest networkRequest = params.addUserAgent(request); + static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, Builder params) { + FullHttpRequest networkRequest = addUserAgent(params.userAgent, request); Origin origin = originFromRequest(networkRequest, params.https()); - SslContext sslContext = getSslContext(params.https(), params.tlsSettings().orElse(null)); + SslContext sslContext = getSslContext(params.https(), params.tlsSettings()); Observable responseObservable = connectionFactory.createConnection( origin, - new ConnectionSettings(params.connectionSettings()), + new ConnectionSettings(params.connectTimeoutMillis()), sslContext ).flatMap(connection -> connection.write(networkRequest.toStreamingRequest()) @@ -93,9 +92,20 @@ static CompletableFuture sendRequestInternal(NettyConnectionFa } + private static FullHttpRequest addUserAgent(String userAgent, FullHttpRequest request) { + if (userAgent != null) { + return request.newBuilder() + .header(USER_AGENT, userAgent) + .build(); + } else { + return request; + } + } + + private static SslContext getSslContext(boolean isHttps, TlsSettings tlsSettings) { if (isHttps) { - return Optional.of(tlsSettings != null ? tlsSettings : TransactionParameters.DEFAULT_TLS_SETTINGS) + return Optional.of(tlsSettings != null ? tlsSettings : new TlsSettings.Builder().build()) .map(SslContextFactory::get) .orElse(null); } else { @@ -136,6 +146,29 @@ public static class Builder { private String threadName = "simple-netty-http-client"; private boolean isHttps; + public Builder() { + + } + +// public Builder(TransactionParameters parameters) { +// this.connectTimeoutMillis = parameters.connectionTimeoutMillis(); +// this.maxResponseSize = parameters.maxResponseSize(); +// this.tlsSettings = parameters.tlsSettings().orElse(new TlsSettings.Builder().build()); +// this.responseTimeout = parameters.responseTimeout(); +// this.isHttps = parameters.https(); +// this.userAgent = parameters.userAgent().orElse(null); +// } + + public Builder(Builder another) { + this.connectTimeoutMillis = another.connectTimeoutMillis; + this.maxResponseSize = another.maxResponseSize; + this.tlsSettings = another.tlsSettings; + this.responseTimeout = another.responseTimeout; + this.isHttps = another.isHttps; + this.userAgent = another.userAgent; + this.maxHeaderSize = another.maxHeaderSize; + } + public Builder connectTimeout(int timeoutMs) { this.connectTimeoutMillis = timeoutMs; return this; @@ -207,13 +240,17 @@ public Builder threadName(String threadName) { return this; } + Builder copy() { + return new Builder(this); + } + /** * Construct a client instance. * * @return a new instance */ public StyxHttpClient build() { - connectionFactory = new NettyConnectionFactory.Builder() + NettyConnectionFactory connectionFactory = new NettyConnectionFactory.Builder() .name(threadName) .httpConfig(newHttpConfigBuilder().setMaxHeadersSize(maxHeaderSize).build()) .tlsSettings(tlsSettings) @@ -225,7 +262,9 @@ public StyxHttpClient build() { false, false)) .build(); - return new StyxHttpClient(this); + + return new StyxHttpClient(connectionFactory, this); } + } } diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java index c066625f20..3930740e83 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java @@ -25,8 +25,8 @@ class StyxHttpClientTransaction implements HttpClient.Transaction { private StyxHttpClient.Builder builder; private NettyConnectionFactory connectionFactory; - public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, TransactionParameters parent) { - this.builder = parent.newBuilder(); + public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, StyxHttpClient.Builder builder) { + this.builder = builder; this.connectionFactory = connectionFactory; } @@ -49,6 +49,6 @@ public HttpClient.StreamingTransaction streaming() { @Override public CompletableFuture sendRequest(FullHttpRequest request) { - return StyxHttpClient.sendRequestInternal(connectionFactory, request, new TransactionParameters(builder)); + return StyxHttpClient.sendRequestInternal(connectionFactory, request, builder); } } diff --git a/components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java b/components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java deleted file mode 100644 index ca69e2680c..0000000000 --- a/components/client/src/main/java/com/hotels/styx/client/TransactionParameters.java +++ /dev/null @@ -1,96 +0,0 @@ -/* - Copyright (C) 2013-2018 Expedia 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 com.hotels.styx.client; - -import com.hotels.styx.api.FullHttpRequest; -import com.hotels.styx.api.extension.service.TlsSettings; - -import java.util.Optional; - -import static com.hotels.styx.api.HttpHeaderNames.USER_AGENT; - -class TransactionParameters { - static final TlsSettings DEFAULT_TLS_SETTINGS = new TlsSettings.Builder().build(); - private final int connectTimeoutMillis; - private final Optional userAgent; - private final int maxResponseSize; - private final boolean isHttps; - private final int responseTimeout; - private TlsSettings tlsSettings; - - TransactionParameters(StyxHttpClient.Builder builder) { - this.userAgent = Optional.ofNullable(builder.userAgent()); - this.connectTimeoutMillis = builder.connectTimeoutMillis(); - this.maxResponseSize = builder.maxResponseSize(); - this.isHttps = builder.https(); - this.tlsSettings = builder.tlsSettings(); - this.responseTimeout = builder.responseTimeout(); - } - - Optional userAgent() { - return userAgent; - } - - int connectionSettings() { - return connectTimeoutMillis; - } - - int maxResponseSize() { - return maxResponseSize; - } - - boolean https() { - return isHttps; - } - - String threadName() { - return "SimpleHttpClientThread"; - } - - int responseTimeout() { - return this.responseTimeout; - } - - FullHttpRequest addUserAgent(FullHttpRequest request) { - return userAgent.map(value -> - request.newBuilder() - .header(USER_AGENT, value) - .build()) - .orElse(request); - } - - StyxHttpClient.Builder newBuilder() { - StyxHttpClient.Builder builder = new StyxHttpClient.Builder() - .connectTimeout(connectTimeoutMillis) - .maxHeaderSize(maxResponseSize) - .tlsSettings(Optional.ofNullable(tlsSettings).orElse(DEFAULT_TLS_SETTINGS)) - .responseTimeout(responseTimeout) - .secure(isHttps); - - userAgent.ifPresent(builder::userAgent); - - return builder; - } - - int maxHeaderSize() { - return 8192; - } - - Optional tlsSettings() { - return Optional.ofNullable(tlsSettings); - } - -} diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index c77b57c85b..b430d8cd69 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -305,7 +305,7 @@ public void sendsToDefaultHttpPort() { StyxHttpClient.sendRequestInternal(factory, get("/") .header(HOST, "localhost") .build(), - new TransactionParameters(new StyxHttpClient.Builder())); + new StyxHttpClient.Builder()); verify(factory).createConnection(originCaptor.capture(), any(ConnectionSettings.class), any(SslContext.class)); assertThat(originCaptor.getValue().port(), is(80)); @@ -319,7 +319,7 @@ public void sendsToDefaultHttpsPort() { StyxHttpClient.sendRequestInternal(factory, get("/") .header(HOST, "localhost") .build(), - new TransactionParameters(new StyxHttpClient.Builder().secure(true))); + new StyxHttpClient.Builder().secure(true)); verify(factory).createConnection(originCaptor.capture(), any(ConnectionSettings.class), any(SslContext.class)); assertThat(originCaptor.getValue().port(), is(443)); From c42156e7c3bc11210ca5f0bf4efdfbe950c6498a Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 11:47:40 +0100 Subject: [PATCH 10/15] Improve javadocs. Tidy up. --- .../com/hotels/styx/client/HttpClient.java | 10 +- .../hotels/styx/client/StyxHttpClient.java | 151 +++++++++++++----- .../client/StyxHttpClientTransaction.java | 34 +++- 3 files changed, 140 insertions(+), 55 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java index 2e9eb19684..5aa33db178 100644 --- a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java @@ -54,16 +54,16 @@ interface Transaction { /** * Send the request using TLS protocol. * - * @return this @{code Transaction} object + * @return this {@code Transaction} object */ Transaction secure(); /** * Determines if the request should be sent securely or not. * - * @param secure Set to @{code true} if the request should be sent securely, - * or @{code false} if the request should be sent insecurely. - * @return this @{code Transaction} object + * @param secure Set to {@code true} if the request should be sent securely, + * or {@code false} if the request should be sent insecurely. + * @return this {@code Transaction} object */ Transaction secure(boolean secure); @@ -71,7 +71,7 @@ interface Transaction { * Converts the transaction object to streaming transaction. * * A call to {@code streaming()} converts this {@link Transaction} object to - * a @{link StreamingTransaction}. This allows responses to be consumed + * a {@link StreamingTransaction}. This allows responses to be consumed * in streaming responses. * * @return a {@link StreamingTransaction} object diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 93a24f60f7..9a210d9c84 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -20,6 +20,7 @@ import com.hotels.styx.api.FullHttpRequest; import com.hotels.styx.api.FullHttpResponse; import com.hotels.styx.api.Url; +import com.hotels.styx.api.exceptions.ResponseTimeoutException; import com.hotels.styx.api.extension.Origin; import com.hotels.styx.api.extension.service.TlsSettings; import com.hotels.styx.client.netty.connectionpool.HttpRequestOperation; @@ -55,25 +56,50 @@ private StyxHttpClient(NettyConnectionFactory connectionFactory, Builder paramet this.connectionFactory = connectionFactory; } + /** + * Shuts the styx HTTP client thread pool. + * + * @return A {@link CompletableFuture} that completes when the thread pool is terminated + */ public CompletableFuture shutdown() { return connectionFactory.close(); } + /** + * Indicates that a request should be sent using secure {@code https} protocol. + * + * @return a {@HttpClient.Transaction} instance that allows fluent method chaining + */ public HttpClient.Transaction secure() { return new StyxHttpClientTransaction(connectionFactory, this.transactionParameters.copy().secure(true)); } + /** + * Indicates if a request should be sent over secure {@code https} or insecure {@code http} protocol. + * + * A value of {@code true} indicates that a request should be sent over a secure {@code https} protocol. + * A value of (@code false} indicates that a request should be sent over an insecure {@code http} protocol. + * + * @param secure a boolean flag to indicate if a request should be sent over a secure protocol or not + * @return a {@HttpClient.Transaction} instance that allows fluent method chaining + */ public HttpClient.Transaction secure(boolean secure) { return new StyxHttpClientTransaction(connectionFactory, this.transactionParameters.copy().secure(secure)); } + /** + * Sends a request as {@link FullHttpRequest} object. + * + * @param request a {@link FullHttpRequest} object to be sent to remote origin. + * @return a {@link CompletableFuture} of response + */ public CompletableFuture sendRequest(FullHttpRequest request) { return sendRequestInternal(connectionFactory, request, this.transactionParameters); } @VisibleForTesting static CompletableFuture sendRequestInternal(NettyConnectionFactory connectionFactory, FullHttpRequest request, Builder params) { - FullHttpRequest networkRequest = addUserAgent(params.userAgent, request); + FullHttpRequest networkRequest = addUserAgent(params.userAgent(), request); Origin origin = originFromRequest(networkRequest, params.https()); SslContext sslContext = getSslContext(params.https(), params.tlsSettings()); @@ -136,39 +162,46 @@ private static Origin originFromRequest(FullHttpRequest request, Boolean isHttps * Builder for {@link StyxHttpClient}. */ public static class Builder { - private NettyConnectionFactory connectionFactory; - private TlsSettings tlsSettings; - private String userAgent; + private String threadName = "simple-netty-http-client"; private int connectTimeoutMillis = 1000; - private int responseTimeout = 60000; private int maxResponseSize = 1024 * 100; + private int responseTimeout = 60000; private int maxHeaderSize = 8192; - private String threadName = "simple-netty-http-client"; + private TlsSettings tlsSettings; private boolean isHttps; + private String userAgent; public Builder() { - } -// public Builder(TransactionParameters parameters) { -// this.connectTimeoutMillis = parameters.connectionTimeoutMillis(); -// this.maxResponseSize = parameters.maxResponseSize(); -// this.tlsSettings = parameters.tlsSettings().orElse(new TlsSettings.Builder().build()); -// this.responseTimeout = parameters.responseTimeout(); -// this.isHttps = parameters.https(); -// this.userAgent = parameters.userAgent().orElse(null); -// } - - public Builder(Builder another) { + Builder(Builder another) { + this.threadName = another.threadName; this.connectTimeoutMillis = another.connectTimeoutMillis; this.maxResponseSize = another.maxResponseSize; - this.tlsSettings = another.tlsSettings; this.responseTimeout = another.responseTimeout; + this.maxHeaderSize = another.maxHeaderSize; + this.tlsSettings = another.tlsSettings; this.isHttps = another.isHttps; this.userAgent = another.userAgent; - this.maxHeaderSize = another.maxHeaderSize; } + /** + * Sets a thread name used for the thread pool. + * + * @param threadName thread name + * @return this {@link Builder} + */ + public Builder threadName(String threadName) { + this.threadName = threadName; + return this; + } + + /** + * Sets the TCP connection timeout. + * + * @param timeoutMs desired TCP connection timeout + * @return this {@link Builder} + */ public Builder connectTimeout(int timeoutMs) { this.connectTimeoutMillis = timeoutMs; return this; @@ -178,39 +211,60 @@ int connectTimeoutMillis() { return connectTimeoutMillis; } - public Builder responseTimeout(int responseTimeoutMs) { - this.responseTimeout = responseTimeoutMs; + /** + * Sets the maximum allowed response size in bytes. + * + * @param maxResponseSize maximum response size in bytes. + * @return this {@link Builder} + */ + public Builder maxResponseSize(int maxResponseSize) { + this.maxResponseSize = maxResponseSize; return this; } - int responseTimeout() { - return responseTimeout; + int maxResponseSize() { + return this.maxResponseSize; } /** - * Sets the user-agent header value to be included in requests. + * Maximum time in milliseconds this client is willing to wait for the origin server to respond. * - * @param userAgent user-agent - * @return this builder + * Sets a maximum tolerated length of inactivity on TCP connection before remote origin is considered + * unresponsive. After this time a {@link ResponseTimeoutException} is thrown is emitted on the + * response future. + * + * Note that an actual response can take considerably longer time to arrive than @{code responseTimeoutMillis}. + * This can happen if origin sends the response slowly. Origin may send headers first, and then + * slowly drip feed the response body. This is acceptable as long as the TCP connection does not + * experience longer inactivity than @{code responseTimeoutMillis} between any two consecutive + * data packets. + * + * @param responseTimeoutMs maximum tolerated inactivity on the TCP connection. + * @return this {@link Builder} */ - public Builder userAgent(String userAgent) { - this.userAgent = userAgent; + public Builder responseTimeout(int responseTimeoutMs) { + this.responseTimeout = responseTimeoutMs; return this; } - String userAgent() { - return this.userAgent; - } - - public Builder maxResponseSize(int maxResponseSize) { - this.maxResponseSize = maxResponseSize; + /** + * Sets the maximum length for HTTP request or status line. + * + * @param maxHeaderSize maximum HTTP request or status line length in bytes + * @return this {@link Builder} + */ + public Builder maxHeaderSize(int maxHeaderSize) { + this.maxHeaderSize = maxHeaderSize; return this; } - int maxResponseSize() { - return this.maxResponseSize; - } - + /** + * Sets the TLS parameters to be used with secure connections. + * Implies that requests should be sent securely over @{code https} protocol. + * + * @param tlsSettings TLS parameters + * @return this {@link Builder} + */ public Builder tlsSettings(TlsSettings tlsSettings) { this.tlsSettings = requireNonNull(tlsSettings); this.isHttps = true; @@ -221,6 +275,12 @@ TlsSettings tlsSettings() { return this.tlsSettings; } + /** + * Specifies whether requests should be sent securely or not. + * + * @param secure @{code true} if secure {@code https} protocol should be used, or {@code false} if insecure {@code http} protocol can be used + * @return this {@link Builder} + */ public Builder secure(boolean secure) { this.isHttps = secure; return this; @@ -230,14 +290,19 @@ boolean https() { return this.isHttps; } - public Builder maxHeaderSize(int maxHeaderSize) { - this.maxHeaderSize = maxHeaderSize; + /** + * Sets the user-agent header value to be included in requests. + * + * @param userAgent user-agent + * @return this {@link Builder} + */ + public Builder userAgent(String userAgent) { + this.userAgent = userAgent; return this; } - public Builder threadName(String threadName) { - this.threadName = threadName; - return this; + String userAgent() { + return this.userAgent; } Builder copy() { diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java index 3930740e83..d884950e8a 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java @@ -22,33 +22,53 @@ import java.util.concurrent.CompletableFuture; class StyxHttpClientTransaction implements HttpClient.Transaction { - private StyxHttpClient.Builder builder; + private StyxHttpClient.Builder transactionParameters; private NettyConnectionFactory connectionFactory; - public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, StyxHttpClient.Builder builder) { - this.builder = builder; + public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, StyxHttpClient.Builder transactionParameters) { + this.transactionParameters = transactionParameters; this.connectionFactory = connectionFactory; } + /** + * Indicates that a request should be sent using secure {@code https} protocol. + * + * @return a @{HttpClient.Transaction} instance that allows fluent method chaining + */ @Override public HttpClient.Transaction secure() { - this.builder.secure(true); + this.transactionParameters.secure(true); return this; } + /** + * Indicates if a request should be sent over secure {@code https} or insecure {@code http} protocol. + * + * A value of {@code true} indicates that a request should be sent over a secure {@code https} protocol. + * A value of {@code false} indicates that a request should be sent over an insecure {@code http} protocol. + * + * @param secure a boolean flag to indicate if a request should be sent over a secure protocol or not + * @return a @{HttpClient.Transaction} instance that allows fluent method chaining + */ @Override public HttpClient.Transaction secure(boolean secure) { - this.builder.secure(secure); + this.transactionParameters.secure(secure); return this; } @Override public HttpClient.StreamingTransaction streaming() { - return null; + throw new UnsupportedOperationException("Not yet implemented."); } + /** + * Sends a request as {@link FullHttpRequest} object. + * + * @param request a {@link FullHttpRequest} object to be sent to remote origin. + * @return a {@link CompletableFuture} of response + */ @Override public CompletableFuture sendRequest(FullHttpRequest request) { - return StyxHttpClient.sendRequestInternal(connectionFactory, request, builder); + return StyxHttpClient.sendRequestInternal(connectionFactory, request, transactionParameters); } } From e250387b38b171216b4d6f70539f74a074b6ad7d Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 12:04:19 +0100 Subject: [PATCH 11/15] Tidy up. --- .../com/hotels/styx/client/HttpClient.java | 34 +++++++++++++++---- .../hotels/styx/client/StyxHttpClient.java | 7 ++-- .../client/StyxHttpClientTransaction.java | 2 +- .../styx/client/StyxHttpClientTest.java | 26 +++++++------- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java index 5aa33db178..cea8c82be9 100644 --- a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java @@ -31,8 +31,19 @@ * messages. */ public interface HttpClient { -// CompletableFuture sendRequest(HttpRequest request); + /** + * Sends a HTTP request message using this client. + * + * @deprecated use {@link this::send} instead. + * + * @param request a full HTTP request object + * @return a future of full HTTP request object + */ + @Deprecated + default CompletableFuture sendRequest(FullHttpRequest request) { + return send(request); + } /** * Sends a HTTP request message using this client. @@ -40,7 +51,7 @@ public interface HttpClient { * @param request a full HTTP request object * @return a future of full HTTP request object */ - CompletableFuture sendRequest(FullHttpRequest request); + CompletableFuture send(FullHttpRequest request); /** * A HTTP request transaction. @@ -78,7 +89,18 @@ interface Transaction { */ StreamingTransaction streaming(); -// CompletableFuture sendRequest(HttpRequest request); + /** + * Sends a HTTP request message using this client. + * + * @deprecated use {@link this::send} instead. + * + * @param request a full HTTP request object + * @return a future of full HTTP request object + */ + @Deprecated + default CompletableFuture sendRequest(FullHttpRequest request) { + return send(request); + } /** * Sends a HTTP request message using this client. @@ -86,7 +108,7 @@ interface Transaction { * @param request a full HTTP request object * @return a future of full HTTP request object */ - CompletableFuture sendRequest(FullHttpRequest request); + CompletableFuture send(FullHttpRequest request); } /** @@ -96,7 +118,7 @@ interface Transaction { * fashion instead of being aggregated into a FullHttpResponse. */ interface StreamingTransaction { - CompletableFuture sendRequest(HttpRequest request); - CompletableFuture sendRequest(FullHttpRequest request); + CompletableFuture send(HttpRequest request); + CompletableFuture send(FullHttpRequest request); } } diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 9a210d9c84..eaed93a857 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -20,7 +20,6 @@ import com.hotels.styx.api.FullHttpRequest; import com.hotels.styx.api.FullHttpResponse; import com.hotels.styx.api.Url; -import com.hotels.styx.api.exceptions.ResponseTimeoutException; import com.hotels.styx.api.extension.Origin; import com.hotels.styx.api.extension.service.TlsSettings; import com.hotels.styx.client.netty.connectionpool.HttpRequestOperation; @@ -93,7 +92,7 @@ public HttpClient.Transaction secure(boolean secure) { * @param request a {@link FullHttpRequest} object to be sent to remote origin. * @return a {@link CompletableFuture} of response */ - public CompletableFuture sendRequest(FullHttpRequest request) { + public CompletableFuture send(FullHttpRequest request) { return sendRequestInternal(connectionFactory, request, this.transactionParameters); } @@ -230,8 +229,8 @@ int maxResponseSize() { * Maximum time in milliseconds this client is willing to wait for the origin server to respond. * * Sets a maximum tolerated length of inactivity on TCP connection before remote origin is considered - * unresponsive. After this time a {@link ResponseTimeoutException} is thrown is emitted on the - * response future. + * unresponsive. After this time a {@link com.hotels.styx.api.exceptions.ResponseTimeoutException} is + * thrown is emitted on the response future. * * Note that an actual response can take considerably longer time to arrive than @{code responseTimeoutMillis}. * This can happen if origin sends the response slowly. Origin may send headers first, and then diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java index d884950e8a..94fd399ac5 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java @@ -68,7 +68,7 @@ public HttpClient.StreamingTransaction streaming() { * @return a {@link CompletableFuture} of response */ @Override - public CompletableFuture sendRequest(FullHttpRequest request) { + public CompletableFuture send(FullHttpRequest request) { return StyxHttpClient.sendRequestInternal(connectionFactory, request, transactionParameters); } } diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index b430d8cd69..0792522b96 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -86,7 +86,7 @@ public void closesThreadPoolAfterUse() throws InterruptedException, ExecutionExc .build(); // Ensures that a thread is created before the assertions - client.sendRequest(httpRequest).get(); + client.send(httpRequest).get(); assertThat(threadExists("test-client"), is(true)); @@ -109,7 +109,7 @@ public void usesDefaultUserAgent() throws ExecutionException, InterruptedExcepti .build(); FullHttpResponse response = client - .sendRequest(httpRequest) + .send(httpRequest) .get(); assertThat(response.status(), is(OK)); @@ -124,7 +124,7 @@ public void doesNotSetAnyUserAgentIfNotSpecified() throws ExecutionException, In StyxHttpClient client = new StyxHttpClient.Builder() .build(); - client.sendRequest(httpRequest).get(); + client.send(httpRequest).get(); server.verify( getRequestedFor(urlEqualTo("/")) @@ -143,7 +143,7 @@ public void replacesUserAgentIfAlreadyPresentInRequest() throws ExecutionExcepti .header(USER_AGENT, "My previous user agent") .build(); - client.sendRequest(request).get(); + client.send(request).get(); server.verify( getRequestedFor(urlEqualTo("/")) @@ -159,7 +159,7 @@ public void usesDefaultTlsSettings() throws ExecutionException, InterruptedExcep .build(); FullHttpResponse response = client - .sendRequest(secureRequest) + .send(secureRequest) .get(); assertThat(response.status(), is(OK)); @@ -172,7 +172,7 @@ public void overridesTlsSettingsWithSecure() throws ExecutionException, Interrup FullHttpResponse response = client .secure() - .sendRequest(secureRequest) + .send(secureRequest) .get(); assertThat(response.status(), is(OK)); @@ -185,7 +185,7 @@ public void overridesTlsSettingsWithSecureBoolean() throws ExecutionException, I FullHttpResponse response = client .secure(true) - .sendRequest(secureRequest) + .send(secureRequest) .get(); assertThat(response.status(), is(OK)); @@ -199,7 +199,7 @@ public void overridesTlsSettingsWithSecureBooleanFalse() throws ExecutionExcepti FullHttpResponse response = client .secure(false) - .sendRequest(httpRequest) + .send(httpRequest) .get(); assertThat(response.status(), is(OK)); @@ -221,7 +221,7 @@ public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Thro try { new StyxHttpClient.Builder() .build() - .sendRequest(get("/foo.txt").header(HOST, "a.b.c").build()).get(); + .send(get("/foo.txt").header(HOST, "a.b.c").build()).get(); } catch (ExecutionException cause) { throw cause.getCause(); } @@ -231,7 +231,7 @@ public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Thro public void sendsMessagesInOriginUrlFormat() throws ExecutionException, InterruptedException { FullHttpResponse response = new StyxHttpClient.Builder() .build() - .sendRequest(get("/index.html").header(HOST, hostString(server.port())).build()) + .send(get("/index.html").header(HOST, hostString(server.port())).build()) .get(); assertThat(response.status(), is(OK)); @@ -254,7 +254,7 @@ public void defaultResponseTimeout() throws Throwable { )); try { - client.sendRequest( + client.send( get("/slowResponse") .header(HOST, hostString(server.port())) .build()) @@ -274,7 +274,7 @@ public void defaultResponseTimeout() throws Throwable { public void sendsMessagesInAbsoluteUrlFormat() throws ExecutionException, InterruptedException { FullHttpResponse response = new StyxHttpClient.Builder() .build() - .sendRequest(get(format("http://%s/index.html", hostString(server.port()))).build()) + .send(get(format("http://%s/index.html", hostString(server.port()))).build()) .get(); assertThat(response.status(), is(OK)); @@ -294,7 +294,7 @@ public void requestWithNoHostOrUrlAuthorityCausesException() { StyxHttpClient client = new StyxHttpClient.Builder().build(); - await(client.sendRequest(request)); + await(client.send(request)); } @Test From 3ee78d0c7141d939d917f7291aac1f7cadd01ed7 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 14:55:59 +0100 Subject: [PATCH 12/15] Address code review comments. --- .../com/hotels/styx/client/HttpClient.java | 13 ------- .../hotels/styx/client/StyxHttpClient.java | 2 +- .../client/StyxHttpClientTransaction.java | 4 +- .../netty/connectionpool/NettyConnection.java | 3 +- .../styx/client/StyxHttpClientTest.java | 38 +++++++++---------- 5 files changed, 23 insertions(+), 37 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java index cea8c82be9..607b332b4b 100644 --- a/components/client/src/main/java/com/hotels/styx/client/HttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/HttpClient.java @@ -89,19 +89,6 @@ interface Transaction { */ StreamingTransaction streaming(); - /** - * Sends a HTTP request message using this client. - * - * @deprecated use {@link this::send} instead. - * - * @param request a full HTTP request object - * @return a future of full HTTP request object - */ - @Deprecated - default CompletableFuture sendRequest(FullHttpRequest request) { - return send(request); - } - /** * Sends a HTTP request message using this client. * diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index eaed93a857..4296a1fbd1 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -327,7 +327,7 @@ public StyxHttpClient build() { false)) .build(); - return new StyxHttpClient(connectionFactory, this); + return new StyxHttpClient(connectionFactory, this.copy()); } } diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java index 94fd399ac5..fd95a16955 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClientTransaction.java @@ -22,8 +22,8 @@ import java.util.concurrent.CompletableFuture; class StyxHttpClientTransaction implements HttpClient.Transaction { - private StyxHttpClient.Builder transactionParameters; - private NettyConnectionFactory connectionFactory; + private final StyxHttpClient.Builder transactionParameters; + private final NettyConnectionFactory connectionFactory; public StyxHttpClientTransaction(NettyConnectionFactory connectionFactory, StyxHttpClient.Builder transactionParameters) { this.transactionParameters = transactionParameters; diff --git a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java index caa0df8cb9..9a8c3b9d58 100644 --- a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java +++ b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java @@ -58,8 +58,7 @@ public class NettyConnection implements Connection, TimeToFirstByteListener { * @param channel the netty channel used * @param requestOperationFactory used to create operation objects that send http requests via this connection */ - @VisibleForTesting - public NettyConnection(Origin origin, Channel channel, HttpRequestOperationFactory requestOperationFactory, HttpConfig httpConfig, SslContext sslContext) { + public NettyConnection(Origin origin, Channel channel, HttpRequestOperationFactory requestOperationFactory, HttpConfig httpConfig, SslContext sslContext) { this.origin = requireNonNull(origin); this.channel = requireNonNull(channel); this.requestOperationFactory = requestOperationFactory; diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index 0792522b96..d397660359 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -45,6 +45,7 @@ import static com.hotels.styx.common.StyxFutures.await; import static com.hotels.styx.support.server.UrlMatchingStrategies.urlStartingWith; import static java.lang.String.format; +import static java.lang.Thread.getAllStackTraces; import static java.util.concurrent.TimeUnit.SECONDS; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -96,10 +97,24 @@ public void closesThreadPoolAfterUse() throws InterruptedException, ExecutionExc } private static Boolean threadExists(String threadName) { - for (Thread t : Thread.getAllStackTraces().keySet()) { - if (t.getName().startsWith(threadName)) return true; - } - return false; + return getAllStackTraces().keySet().stream() + .anyMatch(thread -> + thread.getName().startsWith(threadName)); + } + + @Test + public void cannotBeModifiedAfterCreated() throws ExecutionException, InterruptedException { + StyxHttpClient.Builder builder = new StyxHttpClient.Builder().userAgent("v1"); + + StyxHttpClient client = builder.build(); + + builder.userAgent("v2"); + + assertThat(client.send(httpRequest).get().status(), is(OK)); + server.verify( + getRequestedFor(urlEqualTo("/")) + .withHeader("User-Agent", equalTo("v1")) + ); } @Test @@ -212,21 +227,6 @@ public void requiresValidTlsSettins() { .build(); } - // TODO: Mikko: - // Note: this test case might fail depending on your ISP's configuration. - // Some ISPs may redirect failed DNS lookups to a specific landing page for - // advertising purposes. - @Test(expectedExceptions = OriginUnreachableException.class) - public void throwsOriginUnreachableExceptionWhenDnsResolutionFails() throws Throwable { - try { - new StyxHttpClient.Builder() - .build() - .send(get("/foo.txt").header(HOST, "a.b.c").build()).get(); - } catch (ExecutionException cause) { - throw cause.getCause(); - } - } - @Test public void sendsMessagesInOriginUrlFormat() throws ExecutionException, InterruptedException { FullHttpResponse response = new StyxHttpClient.Builder() From 25c7f1f778af60b79112620305d1f6ce89fc744f Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 15:09:43 +0100 Subject: [PATCH 13/15] Address code review comments. --- .../com/hotels/styx/client/StyxHttpClient.java | 8 +++++--- .../netty/connectionpool/NettyConnection.java | 6 ++---- .../connectionpool/NettyConnectionFactory.java | 12 ++---------- .../hotels/styx/common/CompletableFutures.java | 15 +++++++++++++++ 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index 4296a1fbd1..eb85f44333 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -30,6 +30,7 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; import static com.google.common.base.Preconditions.checkArgument; import static com.hotels.styx.api.HttpHeaderNames.HOST; @@ -198,11 +199,12 @@ public Builder threadName(String threadName) { /** * Sets the TCP connection timeout. * - * @param timeoutMs desired TCP connection timeout + * @param duration desired TCP connection timeout duration + * @param timeUnit duration unit * @return this {@link Builder} */ - public Builder connectTimeout(int timeoutMs) { - this.connectTimeoutMillis = timeoutMs; + public Builder connectTimeout(int duration, TimeUnit timeUnit) { + this.connectTimeoutMillis = (int) timeUnit.toMillis(duration); return this; } diff --git a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java index 9a8c3b9d58..88c5946675 100644 --- a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java +++ b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnection.java @@ -15,15 +15,13 @@ */ package com.hotels.styx.client.netty.connectionpool; -import com.google.common.annotations.VisibleForTesting; -import com.hotels.styx.api.extension.Announcer; import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; -import com.hotels.styx.client.Connection; +import com.hotels.styx.api.extension.Announcer; import com.hotels.styx.api.extension.Origin; +import com.hotels.styx.client.Connection; import com.hotels.styx.client.HttpConfig; import com.hotels.styx.client.HttpRequestOperationFactory; - import io.netty.channel.Channel; import io.netty.channel.ChannelPipeline; import io.netty.handler.codec.http.HttpClientCodec; diff --git a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java index 19113c1bd0..1382a7329a 100644 --- a/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java +++ b/components/client/src/main/java/com/hotels/styx/client/netty/connectionpool/NettyConnectionFactory.java @@ -25,6 +25,7 @@ import com.hotels.styx.client.HttpRequestOperationFactory; import com.hotels.styx.client.netty.eventloop.PlatformAwareClientEventLoopGroupFactory; import com.hotels.styx.client.ssl.SslContextFactory; +import com.hotels.styx.common.CompletableFutures; import io.netty.bootstrap.Bootstrap; import io.netty.buffer.PooledByteBufAllocator; import io.netty.channel.Channel; @@ -109,16 +110,7 @@ private synchronized void bootstrap(ConnectionSettings connectionSettings) { } public CompletableFuture close() { - CompletableFuture future = new CompletableFuture(); - eventLoopGroup.shutdownGracefully() - .addListener(nettyFuture -> { - if (nettyFuture.isSuccess()) { - future.complete(null); - } else { - future.completeExceptionally(nettyFuture.cause()); - } - }); - return future; + return CompletableFutures.fromNettyFuture(eventLoopGroup.shutdownGracefully()); } private class Initializer extends ChannelInitializer { diff --git a/components/common/src/main/java/com/hotels/styx/common/CompletableFutures.java b/components/common/src/main/java/com/hotels/styx/common/CompletableFutures.java index 969c90569c..b1ec2cb274 100644 --- a/components/common/src/main/java/com/hotels/styx/common/CompletableFutures.java +++ b/components/common/src/main/java/com/hotels/styx/common/CompletableFutures.java @@ -15,6 +15,7 @@ */ package com.hotels.styx.common; +import io.netty.util.concurrent.Future; import rx.Observable; import java.util.concurrent.CompletableFuture; @@ -32,4 +33,18 @@ public static CompletableFuture fromSingleObservable(Observable observ return future; } + public static CompletableFuture fromNettyFuture(Future nettyFuture) { + CompletableFuture future = new CompletableFuture<>(); + + nettyFuture.addListener(it -> { + if (it.isSuccess()) { + future.complete(null); + } else { + future.completeExceptionally(it.cause()); + } + }); + + return future; + } + } From 54bb4923df60e2840b3035690b6a4d1984f27efc Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 15:35:30 +0100 Subject: [PATCH 14/15] Fix errors. --- .../styx/proxy/BackendServicesRouter.java | 17 +++++++++-------- .../com/hotels/styx/StyxClientSupplier.scala | 6 ++++-- .../scala/com/hotels/styx/proxy/ProxySpec.scala | 2 +- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java b/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java index ad7cc97450..61281b2881 100644 --- a/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java +++ b/components/proxy/src/main/java/com/hotels/styx/proxy/BackendServicesRouter.java @@ -16,27 +16,27 @@ package com.hotels.styx.proxy; import com.hotels.styx.Environment; -import com.hotels.styx.client.BackendServiceClient; import com.hotels.styx.api.HttpHandler; import com.hotels.styx.api.HttpInterceptor; import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.Id; -import com.hotels.styx.api.StyxObservable; -import com.hotels.styx.client.Connection; -import com.hotels.styx.client.ConnectionSettings; -import com.hotels.styx.client.connectionpool.ConnectionPool; -import com.hotels.styx.api.extension.service.ConnectionPoolSettings; import com.hotels.styx.api.MetricRegistry; +import com.hotels.styx.api.StyxObservable; import com.hotels.styx.api.extension.service.BackendService; +import com.hotels.styx.api.extension.service.ConnectionPoolSettings; import com.hotels.styx.api.extension.service.HealthCheckConfig; import com.hotels.styx.api.extension.service.TlsSettings; import com.hotels.styx.api.extension.service.spi.Registry; +import com.hotels.styx.client.BackendServiceClient; +import com.hotels.styx.client.Connection; +import com.hotels.styx.client.ConnectionSettings; import com.hotels.styx.client.OriginStatsFactory; import com.hotels.styx.client.OriginsInventory; -import com.hotels.styx.client.StyxHttpClient; import com.hotels.styx.client.StyxHeaderConfig; import com.hotels.styx.client.StyxHostHttpClient; +import com.hotels.styx.client.StyxHttpClient; +import com.hotels.styx.client.connectionpool.ConnectionPool; import com.hotels.styx.client.connectionpool.ConnectionPoolFactory; import com.hotels.styx.client.connectionpool.ExpiringConnectionFactory; import com.hotels.styx.client.healthcheck.OriginHealthCheckFunction; @@ -58,6 +58,7 @@ import static java.util.Comparator.comparingInt; import static java.util.Comparator.naturalOrder; import static java.util.Objects.requireNonNull; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.slf4j.LoggerFactory.getLogger; /** @@ -216,7 +217,7 @@ private static OriginHealthCheckFunction originHealthCheckFunction( private static StyxHttpClient healthCheckClient(Id appId, Optional tlsSettings, String styxVersion, ConnectionSettings connectionSettings) { StyxHttpClient.Builder builder = new StyxHttpClient.Builder() - .connectTimeout(connectionSettings.connectTimeoutMillis()) + .connectTimeout(connectionSettings.connectTimeoutMillis(), MILLISECONDS) .threadName("Health-Check-Monitor-" + appId) .userAgent("Styx/" + styxVersion); diff --git a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala index e728087e42..bf49e5730c 100644 --- a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala +++ b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/StyxClientSupplier.scala @@ -15,6 +15,8 @@ */ package com.hotels.styx +import java.util.concurrent.TimeUnit.MILLISECONDS + import com.hotels.styx.api._ import com.hotels.styx.client.StyxHttpClient import org.scalatest.{BeforeAndAfterAll, Suite} @@ -32,7 +34,7 @@ trait StyxClientSupplier extends BeforeAndAfterAll { val client: StyxHttpClient = new StyxHttpClient.Builder() .threadName("scalatest-e2e-client") - .connectTimeout(1000) + .connectTimeout(1000, MILLISECONDS) .maxHeaderSize(2 * 8192) .build() @@ -42,7 +44,7 @@ trait StyxClientSupplier extends BeforeAndAfterAll { } private def doRequest(request: FullHttpRequest, secure: Boolean = false): Future[FullHttpResponse] = if (secure) - client.secure().sendRequest(request).toScala + client.secure().send(request).toScala else client.sendRequest(request).toScala diff --git a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala index ea7887d674..c77ab44903 100644 --- a/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala +++ b/system-tests/e2e-suite/src/test/scala/com/hotels/styx/proxy/ProxySpec.scala @@ -124,7 +124,7 @@ class ProxySpec extends FunSpec it("should respond to HEAD with bodiless response") { val client: HttpClient = new StyxHttpClient.Builder() - .connectTimeout(1000) + .connectTimeout(1000, MILLISECONDS) .maxHeaderSize(2 * 8192) .threadName("scalatest-e2e-client") .build() From 4a6cf843a32b768d17d053eb0876aa1ed462a721 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Tue, 18 Sep 2018 16:25:00 +0100 Subject: [PATCH 15/15] Add timeUnit to 'responseTimeout' method. --- .../main/java/com/hotels/styx/client/StyxHttpClient.java | 7 ++++--- .../java/com/hotels/styx/client/StyxHttpClientTest.java | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java index eb85f44333..cbe66a5976 100644 --- a/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/StyxHttpClient.java @@ -240,11 +240,12 @@ int maxResponseSize() { * experience longer inactivity than @{code responseTimeoutMillis} between any two consecutive * data packets. * - * @param responseTimeoutMs maximum tolerated inactivity on the TCP connection. + * @param duration maximum tolerated inactivity on the TCP connection. + * @param timeUnit time unit for @{code duration} * @return this {@link Builder} */ - public Builder responseTimeout(int responseTimeoutMs) { - this.responseTimeout = responseTimeoutMs; + public Builder responseTimeout(int duration, TimeUnit timeUnit) { + this.responseTimeout = (int) timeUnit.toMillis(duration); return this; } diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java index d397660359..b910c1c4e6 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java @@ -244,7 +244,7 @@ public void sendsMessagesInOriginUrlFormat() throws ExecutionException, Interrup @Test(expectedExceptions = ResponseTimeoutException.class) public void defaultResponseTimeout() throws Throwable { StyxHttpClient client = new StyxHttpClient.Builder() - .responseTimeout(1000) + .responseTimeout(1, SECONDS) .build(); server.stubFor(WireMock.get(urlStartingWith("/slowResponse"))