michael-o commented on a change in pull request #206: URL: https://github.com/apache/httpcomponents-core/pull/206#discussion_r461634116
########## 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: Why should it deadlock? As far as I understand RFC 7230 the server should close the connection in such a case and I client should monitor here. If the client does not monitor, it will recieve a `SocketException`. ########## 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: You cannot change this public interface imho. ########## 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: Strictly speaking, this should be a separate PR. ########## 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: Didn't you say you lose 8 MiB/s on a 100 MBit/s link? ########## 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/[email protected]/msg09911.html";>msg09911</a> + * @see <a href="https://issues.apache.org/jira/browse/HTTPCORE-639";>HTTPCORE-639</a> Review comment: This is part of the commit title. Should not be necessary here. ########## 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: Opposed to the JIRA ticket, the Socket is gone. Why? ########## 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 Review comment: `8kb` > `8 KiB` ########## 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: Why `checks`? We have only a single one, don't we? ########## 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 do not fully understand the test. How do you check that the server has prematurely closed the request processing? ########## 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: I do not really under stand this. How does the Socket API refactoring affect this?! How can a socket input stream be a `FileInputStream`. Can we really move this change bit to a seperate PR please? I'd like to focus on the new interface first. ########## 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/[email protected]/msg09911.html";>msg09911</a> Review comment: Should be in the commit message. ########## 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/[email protected]/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: While this seems to be fine Java-wise, we have never applied this enum pattern to be immutable elsewhere. ########## 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: If the interface defines bytes there is no need to reproduce it in the method name. ########## 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 am not really happy with the name for now, thinking of a better one... ########## 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: Sorry for the confusion. The `toString` isn't the problem. Although, I see little benefit over the autogenerated one, but the builder introduction. This should be a separate PR. ########## 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: You are right, I see it now. The previous constructor remains `public`. ########## 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/[email protected]/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: Please have a look at existing default strategies and apply the same pattern, even if it is suboptimal, but at least consistent as you have said. ########## 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: @ok2c WDYT, do we need the socket here? ########## 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: I was actually hinting to plural vs. singular. There is a single check. ########## 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: Yes, please undo this change here. I will go over to #207 when this one is merged. ---------------------------------------------------------------- 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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
