You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by GitBox <gi...@apache.org> on 2020/07/29 07:28:14 UTC
[GitHub] [httpcomponents-core] carterkozak commented on a change in pull request #206: HTTPCORE-639: Configurable ResponseOutOfOrderStrategy
carterkozak commented on a change in pull request #206:
URL: https://github.com/apache/httpcomponents-core/pull/206#discussion_r461662305
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultBHttpClientConnectionFactory.java
##########
@@ -55,25 +56,44 @@
private final CharCodingConfig charCodingConfig;
private final ContentLengthStrategy incomingContentStrategy;
private final ContentLengthStrategy outgoingContentStrategy;
+ private final ResponseOutOfOrderStrategy responseOutOfOrderStrategy;
private final HttpMessageWriterFactory<ClassicHttpRequest> requestWriterFactory;
private final HttpMessageParserFactory<ClassicHttpResponse> responseParserFactory;
- public DefaultBHttpClientConnectionFactory(
Review comment:
Sorry for the confusion, the github diff is misleading. This private constructor is completely new, this method takes the additional ResponseOutOfOrderStrategy field. I opted to make it private so we can add future fields to the builder and we aren't forced to grow beyond five constructors.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultBHttpClientConnectionFactory.java
##########
@@ -100,10 +120,87 @@ public DefaultBHttpClientConnection createConnection(final Socket socket) throws
CharCodingSupport.createEncoder(this.charCodingConfig),
this.incomingContentStrategy,
this.outgoingContentStrategy,
+ this.responseOutOfOrderStrategy,
this.requestWriterFactory,
this.responseParserFactory);
conn.bind(socket);
return conn;
}
+ @Override
+ public String toString() {
Review comment:
I can remove this if you like, would you prefer a separate ticket for `DefaultBHttpClientConnectionFactory.toString()`?
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,54 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.impl.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+import org.apache.hc.core5.http.io.ResponseOutOfOrderStrategy;
+
+/**
+ * Default {@link ResponseOutOfOrderStrategy} implementation which checks for premature responses every 8kb based
+ * on testing using values between 4 KiB and 128 KiB. This is optimized for correctness, and results in a maximum
+ * upload speed of 8 MiB/s.
+ *
+ * @see <a href="https://www.mail-archive.com/httpclient-users@hc.apache.org/msg09911.html">msg09911</a>
+ * @see <a href="https://issues.apache.org/jira/browse/HTTPCORE-639">HTTPCORE-639</a>
+ * @since 5.1
+ */
+public enum DefaultResponseOutOfOrderStrategy implements ResponseOutOfOrderStrategy {
Review comment:
Happy to migrate this to a final class with a private constructor if you prefer. I tend to prefer enum singletons because it's not necessary to define a private constructor and they cannot be reflectively instantiated, but I agree that it's best to be consistent across the codebase.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/io/ResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,47 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+
+/**
+ * Represents a strategy to determine how frequently the client should check for an out of order response.
+ * An out of order response is sent before the server has read the full request, which may result in the
+ * request becoming deadlocked and eventually timing out.
+ *
+ * @since 5.1
+ */
+public interface ResponseOutOfOrderStrategy {
+
+ /**
+ * Returns the interval in bytes describing how often the socket is checked for an out-of-order response.
+ * A value of zero results in checks being disabled, this may result in requests blocking until the socket
+ * timeout is reached while a server sends an out-of-order response.
+ */
+ int determineResponseCheckIntervalBytes(ClassicHttpRequest request);
Review comment:
Based on HTTPCORE-640 I don't think it's helpful to check if the socket is an instance of SSLSocket anymore.
The socket object could be helpful if we want to check the send and receive buffer sizes when they are explicitly set, but I'm not sure exactly how that would work, and in most cases the system defaults are used.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,54 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.impl.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+import org.apache.hc.core5.http.io.ResponseOutOfOrderStrategy;
+
+/**
+ * Default {@link ResponseOutOfOrderStrategy} implementation which checks for premature responses every 8kb based
+ * on testing using values between 4 KiB and 128 KiB. This is optimized for correctness, and results in a maximum
+ * upload speed of 8 MiB/s.
Review comment:
No, unfortunately it limits any link to at most 8 MiB/s. In the good case, the server does not respond until clients have successfully transferred all request bytes. The check must spend 1ms every 8KiB transferred because the full timeout is reached when no bytes are available. Assuming instantaneous transfer speed, the out of order check reduces us to 8KiB per millisecond which is roughly equivalent to 8MiB/second.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/io/ResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,47 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+
+/**
+ * Represents a strategy to determine how frequently the client should check for an out of order response.
+ * An out of order response is sent before the server has read the full request, which may result in the
+ * request becoming deadlocked and eventually timing out.
+ *
+ * @since 5.1
+ */
+public interface ResponseOutOfOrderStrategy {
+
+ /**
+ * Returns the interval in bytes describing how often the socket is checked for an out-of-order response.
+ * A value of zero results in checks being disabled, this may result in requests blocking until the socket
Review comment:
The check runs every 8192 bytes that we transfer.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultBHttpClientConnection.java
##########
@@ -154,64 +197,81 @@ public void sendRequestEntity(final ClassicHttpRequest request) throws HttpExcep
throw new LengthRequiredException();
}
+
try (final OutputStream outStream = createContentOutputStream(
- len, this.outbuffer, new OutputStream() {
+ len,
+ this.outbuffer,
+ createResponseOutOfOrderCheckingOutputStream(socketHolder, request),
+ entity.getTrailers())) {
+ entity.writeTo(outStream);
+ } catch (final ResponseOutOfOrderException ex) {
+ if (len > 0) {
+ this.consistent = false;
+ }
+ }
+ }
- final boolean ssl = socketHolder.getSocket() instanceof SSLSocket;
- final InputStream socketInputStream = socketHolder.getInputStream();
- final OutputStream socketOutputStream = socketHolder.getOutputStream();
+ private OutputStream createResponseOutOfOrderCheckingOutputStream(
+ final SocketHolder socketHolder,
+ final ClassicHttpRequest request) throws IOException {
+ final OutputStream socketOutputStream = socketHolder.getOutputStream();
+ final int responseOutOfOrderIntervalBytes =
+ responseOutOfOrderStrategy.determineResponseCheckIntervalBytes(request);
+ if (responseOutOfOrderIntervalBytes <= 0) {
+ // Response out of order checks are disabled
+ return socketOutputStream;
+ }
- long totalBytes = 0;
- long chunks = -1;
+ return new OutputStream() {
- void checkForEarlyResponse() throws IOException {
- final long n = totalBytes / (8 * 1024);
- if (n > chunks) {
- chunks = n;
- if (ssl ? isDataAvailable(Timeout.ONE_MILLISECOND) : (socketInputStream.available() > 0)) {
- throw new ResponseOutOfOrderException();
- }
- }
- }
+ final InputStream socketInputStream = socketHolder.getInputStream();
+ final boolean requiresRead = !(socketInputStream instanceof FileInputStream);
Review comment:
Sorry Michael, I posted on the jira ticket that I was pulling this part into a separate PR with its own jira ticket:
https://github.com/apache/httpcomponents-core/pull/207
https://issues.apache.org/jira/browse/HTTPCORE-640
Happy to discuss there.
##########
File path: httpcore5-testing/src/test/java/org/apache/hc/core5/testing/classic/ResponseOutOfOrderIntegrationTest.java
##########
@@ -0,0 +1,189 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.testing.classic;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+import org.apache.hc.core5.http.ClassicHttpResponse;
+import org.apache.hc.core5.http.ContentType;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.URIScheme;
+import org.apache.hc.core5.http.impl.bootstrap.HttpRequester;
+import org.apache.hc.core5.http.impl.bootstrap.RequesterBootstrap;
+import org.apache.hc.core5.http.io.HttpRequestHandler;
+import org.apache.hc.core5.http.io.SocketConfig;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.hc.core5.http.io.entity.InputStreamEntity;
+import org.apache.hc.core5.http.message.BasicClassicHttpRequest;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.http.protocol.HttpCoreContext;
+import org.apache.hc.core5.io.CloseMode;
+import org.apache.hc.core5.testing.SSLTestContexts;
+import org.apache.hc.core5.util.Timeout;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExternalResource;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Arrays;
+import java.util.Collection;
+
+@RunWith(Parameterized.class)
+public class ResponseOutOfOrderIntegrationTest {
Review comment:
The server handler sets a response entity without reading any bytes from the request entity. The test fails due to a timeout when we disable the response-out-of-order check.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/io/ResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,47 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+
+/**
+ * Represents a strategy to determine how frequently the client should check for an out of order response.
+ * An out of order response is sent before the server has read the full request, which may result in the
+ * request becoming deadlocked and eventually timing out.
+ *
+ * @since 5.1
+ */
+public interface ResponseOutOfOrderStrategy {
+
+ /**
+ * Returns the interval in bytes describing how often the socket is checked for an out-of-order response.
+ * A value of zero results in checks being disabled, this may result in requests blocking until the socket
Review comment:
Got it, I'll fix this up when I have time in a couple hours!
##########
File path: httpcore5-testing/src/test/java/org/apache/hc/core5/testing/classic/ResponseOutOfOrderIntegrationTest.java
##########
@@ -0,0 +1,189 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.testing.classic;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+import org.apache.hc.core5.http.ClassicHttpResponse;
+import org.apache.hc.core5.http.ContentType;
+import org.apache.hc.core5.http.HttpHost;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.URIScheme;
+import org.apache.hc.core5.http.impl.bootstrap.HttpRequester;
+import org.apache.hc.core5.http.impl.bootstrap.RequesterBootstrap;
+import org.apache.hc.core5.http.io.HttpRequestHandler;
+import org.apache.hc.core5.http.io.SocketConfig;
+import org.apache.hc.core5.http.io.entity.EntityUtils;
+import org.apache.hc.core5.http.io.entity.InputStreamEntity;
+import org.apache.hc.core5.http.message.BasicClassicHttpRequest;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.http.protocol.HttpCoreContext;
+import org.apache.hc.core5.io.CloseMode;
+import org.apache.hc.core5.testing.SSLTestContexts;
+import org.apache.hc.core5.util.Timeout;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExternalResource;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Arrays;
+import java.util.Collection;
+
+@RunWith(Parameterized.class)
+public class ResponseOutOfOrderIntegrationTest {
Review comment:
I'm going to remove this test from this PR in favor of the PR for HTTPCORE-640.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultBHttpClientConnectionFactory.java
##########
@@ -100,10 +120,87 @@ public DefaultBHttpClientConnection createConnection(final Socket socket) throws
CharCodingSupport.createEncoder(this.charCodingConfig),
this.incomingContentStrategy,
this.outgoingContentStrategy,
+ this.responseOutOfOrderStrategy,
this.requestWriterFactory,
this.responseParserFactory);
conn.bind(socket);
return conn;
}
+ @Override
+ public String toString() {
Review comment:
If I remove the builder from this PR I'll need to either sequence the builder PR prior to this one or introduce another public constructor. What do you prefer?
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,54 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.impl.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+import org.apache.hc.core5.http.io.ResponseOutOfOrderStrategy;
+
+/**
+ * Default {@link ResponseOutOfOrderStrategy} implementation which checks for premature responses every 8kb based
+ * on testing using values between 4 KiB and 128 KiB. This is optimized for correctness, and results in a maximum
+ * upload speed of 8 MiB/s.
Review comment:
There's an interesting case where all writes use buffers larger than 8k, which results in a 1ms sleep for each write, rather than 8k chunks. I don't think this can occur using chunked transfer-encoding, but may be problematic for content-length request entities. I will file a separate issue to discuss segmenting writes based on the response check interval.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/io/ResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,47 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+
+/**
+ * Represents a strategy to determine how frequently the client should check for an out of order response.
+ * An out of order response is sent before the server has read the full request, which may result in the
+ * request becoming deadlocked and eventually timing out.
Review comment:
You are correct, based on definitions in [rfc2119](https://www.ietf.org/rfc/rfc2119.txt) servers are not required to do this.
> SHOULD This word, or the adjective "RECOMMENDED", mean that there
may exist valid reasons in particular circumstances to ignore a
particular item, but the full implications must be understood and
carefully weighed before choosing a different course.
I think deadlock was the incorrect word for me to use, the client and server may both block waiting for the other until a timeout is reached (if either has set a timeout).
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/io/ResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,47 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+
+/**
+ * Represents a strategy to determine how frequently the client should check for an out of order response.
+ * An out of order response is sent before the server has read the full request, which may result in the
+ * request becoming deadlocked and eventually timing out.
+ *
+ * @since 5.1
+ */
+public interface ResponseOutOfOrderStrategy {
+
+ /**
+ * Returns the interval in bytes describing how often the socket is checked for an out-of-order response.
+ * A value of zero results in checks being disabled, this may result in requests blocking until the socket
+ * timeout is reached while a server sends an out-of-order response.
+ */
+ int determineResponseCheckIntervalBytes(ClassicHttpRequest request);
Review comment:
I've updated the method name to `checkInterval`.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/io/ResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,48 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+
+/**
+ * Represents a strategy to determine how frequently the client should check for an out of order response.
+ * An out of order response is sent before the server has read the full request. If the client fails to
+ * check for an early response then a {@link java.net.SocketException} or {@link java.net.SocketTimeoutException}
+ * may be thrown while writing the request entity after a timeout is reached on either the client or server.
+ *
+ * @since 5.1
+ */
+public interface ResponseOutOfOrderStrategy {
+
+ /**
+ * Returns the interval in bytes describing how often the socket is checked for an out-of-order response.
+ * A value of zero disables the check. This may result in requests blocking until the socket
+ * timeout is reached while a server sends an out-of-order response.
+ */
+ int checkInterval(ClassicHttpRequest request);
Review comment:
Something else we may want to consider is the ability to check for an early response only once, after the first chunk or two has been sent. I think the most common case if for a server to parse the request and return immediately before consuming content when a precondition fails (a bad credential, for example).
This way we could provide most of the safety without sacrificing throughput for large uploads.
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/DefaultResponseOutOfOrderStrategy.java
##########
@@ -0,0 +1,54 @@
+/*
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ * ====================================================================
+ *
+ * This software consists of voluntary contributions made by many
+ * individuals on behalf of the Apache Software Foundation. For more
+ * information on the Apache Software Foundation, please see
+ * <http://www.apache.org/>.
+ *
+ */
+
+package org.apache.hc.core5.http.impl.io;
+
+import org.apache.hc.core5.http.ClassicHttpRequest;
+import org.apache.hc.core5.http.io.ResponseOutOfOrderStrategy;
+
+/**
+ * Default {@link ResponseOutOfOrderStrategy} implementation which checks for premature responses every 8kb based
+ * on testing using values between 4 KiB and 128 KiB. This is optimized for correctness, and results in a maximum
+ * upload speed of 8 MiB/s.
Review comment:
I have some request log aggregation graphs from an internal production service which shows 40-50mb uploads dropping from 6 seconds p50 to 1 second p50 after I disabled the early response check by subclassing DefaultBHttpClientConnection. I'll try to clean up the graphs so I can post them, I found it interesting that they very closely match the 8 MiB/second napkin math.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org