michael-o commented on a change in pull request #206:
URL:
https://github.com/apache/httpcomponents-core/pull/206#discussion_r462152931
##########
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:
Let's do the builder first and the modify it in this 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:
You are of course right, this early response chunk size should be a
multiple of the buffer size used to write the entity. We cannot access this
buffer size to size dynamically.
I also ask myself whether 2 KiB for chunked TE isn't too small, but this is
another discussion.
##########
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 a client should monitor here. If
the client does not monitor, it will receive a `SocketException`.
##########
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:
Good, can you modify the description then?
##########
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 still not convinced for these reasons:
* Nothing is checked, but calculcated/determined since we pass the request
* Intervals are related to time and not to size
What about `determineResponseCheckChunkSize()`? or `Step`, `Skip`, ...
##########
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:
This is a very good idea. This is what I have observed with Tomcat.
Please also note that the first check might not succeed because BIO doesn't
scale well. It make take upto 5 iterations to detect this. Sometimes even more.
I don't know how to make this reliably. See my outout
http://home.apache.org/~michaelo/issues/early-response/ accompanied with the
known mailing list thread.
----------------------------------------------------------------
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]
