You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2020/07/20 11:24:15 UTC
[GitHub] [cxf] vladikM opened a new pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
vladikM opened a new pull request #684:
URL: https://github.com/apache/cxf/pull/684
----------------------------------------------------------------
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
[GitHub] [cxf] coheigea merged pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
coheigea merged pull request #684:
URL: https://github.com/apache/cxf/pull/684
----------------------------------------------------------------
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
[GitHub] [cxf] vladikM commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
vladikM commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r459035566
##########
File path: rt/transports/http-netty/netty-client/src/main/java/org/apache/cxf/transport/http/netty/client/NettyHttpClientPipelineFactory.java
##########
@@ -44,16 +44,24 @@
private static final Logger LOG =
LogUtils.getL7dLogger(NettyHttpClientPipelineFactory.class);
+
private final TLSClientParameters tlsClientParameters;
private final int readTimeout;
-
+ private final int maxContentLength;
+
public NettyHttpClientPipelineFactory(TLSClientParameters clientParameters) {
this(clientParameters, 0);
}
public NettyHttpClientPipelineFactory(TLSClientParameters clientParameters, int readTimeout) {
+ this(clientParameters, readTimeout, 1048576);
Review comment:
replaced with constant
----------------------------------------------------------------
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
[GitHub] [cxf] coheigea commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
coheigea commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r457935331
##########
File path: core/src/main/java/org/apache/cxf/message/Message.java
##########
@@ -196,6 +196,12 @@
*/
String THREAD_SAFE_STAX_FACTORIES = Message.class.getName() + ".THREAD_SAFE_STAX_FACTORIES";
+
+ /**
+ * Integer property to specify how much bytes client must be able to handle.
+ */
+ String MAX_RESPONSE_CONTENT_LENGTH = Message.class.getName() + ".MAX_RESPONSE_CONTENT_LENGTH";
Review comment:
I think it's probably better to restrict this configuration variable to Netty instead of the CXF Message object, as it only applies to Netty.
##########
File path: rt/transports/http/src/main/java/org/apache/cxf/transport/http/HTTPConduit.java
##########
@@ -652,6 +652,25 @@ protected static int determineConnectionTimeout(Message message,
return (int)ctimeout;
}
+ protected static int determineMaxResponseContentLength(Message message,
Review comment:
Move into Netty subclass.
##########
File path: rt/transports/http/src/main/java/org/apache/cxf/transport/http/HTTPConduit.java
##########
@@ -652,6 +652,25 @@ protected static int determineConnectionTimeout(Message message,
return (int)ctimeout;
}
+ protected static int determineMaxResponseContentLength(Message message,
+ HTTPClientPolicy csPolicy) {
+ Integer maxResponseContentLength = csPolicy.getMaxResponseContentLength();
+ if (message.get(Message.MAX_RESPONSE_CONTENT_LENGTH) != null) {
+ Object obj = message.get(Message.MAX_RESPONSE_CONTENT_LENGTH);
+ try {
+ maxResponseContentLength = Integer.parseInt(obj.toString());
+ } catch (NumberFormatException e) {
+ LOG.log(Level.WARNING, "INVALID_TIMEOUT_FORMAT", new Object[] {
+ Message.MAX_RESPONSE_CONTENT_LENGTH, obj.toString()
+ });
+ }
+ }
+ if (maxResponseContentLength == null) {
+ maxResponseContentLength = Integer.MAX_VALUE;
Review comment:
We should probably default instead to 1048576, which was the previous default. Maybe extract this number to a constant.
##########
File path: rt/transports/http/src/main/resources/schemas/wsdl/http-conf.xsd
##########
@@ -390,6 +390,13 @@
</xs:documentation>
</xs:annotation>
</xs:attribute>
+ <xs:attribute name="MaxResponseContentLength" type="ptp:ParameterizedInt" use="optional" >
Review comment:
I don't think this is required.
----------------------------------------------------------------
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
[GitHub] [cxf] vladikM commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
vladikM commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r457972308
##########
File path: rt/transports/http/src/main/resources/schemas/wsdl/http-conf.xsd
##########
@@ -390,6 +390,13 @@
</xs:documentation>
</xs:annotation>
</xs:attribute>
+ <xs:attribute name="MaxResponseContentLength" type="ptp:ParameterizedInt" use="optional" >
Review comment:
removed
----------------------------------------------------------------
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
[GitHub] [cxf] vladikM commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
vladikM commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r457973233
##########
File path: rt/transports/http/src/main/java/org/apache/cxf/transport/http/HTTPConduit.java
##########
@@ -652,6 +652,25 @@ protected static int determineConnectionTimeout(Message message,
return (int)ctimeout;
}
+ protected static int determineMaxResponseContentLength(Message message,
+ HTTPClientPolicy csPolicy) {
+ Integer maxResponseContentLength = csPolicy.getMaxResponseContentLength();
+ if (message.get(Message.MAX_RESPONSE_CONTENT_LENGTH) != null) {
+ Object obj = message.get(Message.MAX_RESPONSE_CONTENT_LENGTH);
+ try {
+ maxResponseContentLength = Integer.parseInt(obj.toString());
+ } catch (NumberFormatException e) {
+ LOG.log(Level.WARNING, "INVALID_TIMEOUT_FORMAT", new Object[] {
+ Message.MAX_RESPONSE_CONTENT_LENGTH, obj.toString()
+ });
+ }
+ }
+ if (maxResponseContentLength == null) {
+ maxResponseContentLength = Integer.MAX_VALUE;
Review comment:
change default to 1048576, extracted constant
----------------------------------------------------------------
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
[GitHub] [cxf] coheigea commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
coheigea commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r458587539
##########
File path: rt/transports/http-netty/netty-client/src/main/java/org/apache/cxf/transport/http/netty/client/NettyHttpClientPipelineFactory.java
##########
@@ -44,16 +44,24 @@
private static final Logger LOG =
LogUtils.getL7dLogger(NettyHttpClientPipelineFactory.class);
+
private final TLSClientParameters tlsClientParameters;
private final int readTimeout;
-
+ private final int maxContentLength;
+
public NettyHttpClientPipelineFactory(TLSClientParameters clientParameters) {
this(clientParameters, 0);
}
public NettyHttpClientPipelineFactory(TLSClientParameters clientParameters, int readTimeout) {
+ this(clientParameters, readTimeout, 1048576);
Review comment:
Could we just refer to a single constant value (DEFAULT_MAX_RESPONSE_CONTENT_LENGTH)?
##########
File path: rt/transports/http-netty/netty-client/src/main/java/org/apache/cxf/transport/http/netty/client/NettyHttpConduit.java
##########
@@ -651,5 +660,24 @@ public void postShutdown() {
public void preShutdown() {
}
+ protected static int determineMaxResponseContentLength(Message message) {
+ Integer maxResponseContentLength = null;
+ if (message.get(MAX_RESPONSE_CONTENT_LENGTH) != null) {
+ Object obj = message.get(MAX_RESPONSE_CONTENT_LENGTH);
+ try {
+ maxResponseContentLength = Integer.parseInt(obj.toString());
+ } catch (NumberFormatException e) {
+ LOG.log(Level.WARNING, "INVALID_TIMEOUT_FORMAT", new Object[] {
+ MAX_RESPONSE_CONTENT_LENGTH, obj.toString()
+ });
+ }
+ }
+ if (maxResponseContentLength == null) {
+ maxResponseContentLength = SystemPropertyAction.getInteger(MAX_RESPONSE_CONTENT_LENGTH,
+ DEFAULT_MAX_RESPONSE_CONTENT_LENGTH);
+ }
Review comment:
I don't think the system property is necessary, as determineConnection/ReceiveTimeout don't check it, only the message.
----------------------------------------------------------------
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
[GitHub] [cxf] coheigea commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
coheigea commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r457309659
##########
File path: rt/transports/http-netty/netty-client/src/main/java/org/apache/cxf/transport/http/netty/client/NettyHttpClientPipelineFactory.java
##########
@@ -42,18 +43,22 @@
public class NettyHttpClientPipelineFactory extends ChannelInitializer<Channel> {
+ public static final String MAX_CONTENT_LENGTH = "org.apache.cxf.transport.http.netty.aggregator.maxContentLength";
private static final Logger LOG =
LogUtils.getL7dLogger(NettyHttpClientPipelineFactory.class);
+
private final TLSClientParameters tlsClientParameters;
private final int readTimeout;
-
+ private final int maxContentLength;
+
public NettyHttpClientPipelineFactory(TLSClientParameters clientParameters) {
this(clientParameters, 0);
}
public NettyHttpClientPipelineFactory(TLSClientParameters clientParameters, int readTimeout) {
this.tlsClientParameters = clientParameters;
this.readTimeout = readTimeout;
+ this.maxContentLength = SystemPropertyAction.getInteger(MAX_CONTENT_LENGTH, 1048576);
Review comment:
I think it would be better to configure this in the same way as for readTimeout etc., by passing an int value through to this class, and in NettyHttpConduit checking the CXF Message for the value, e.g. see: https://github.com/apache/cxf/blob/b8e24192378aa2b9bd8110186673d8904f6237d5/rt/transports/http/src/main/java/org/apache/cxf/transport/http/HTTPConduit.java#L617
----------------------------------------------------------------
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
[GitHub] [cxf] vladikM commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
vladikM commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r459035425
##########
File path: rt/transports/http-netty/netty-client/src/main/java/org/apache/cxf/transport/http/netty/client/NettyHttpConduit.java
##########
@@ -651,5 +660,24 @@ public void postShutdown() {
public void preShutdown() {
}
+ protected static int determineMaxResponseContentLength(Message message) {
+ Integer maxResponseContentLength = null;
+ if (message.get(MAX_RESPONSE_CONTENT_LENGTH) != null) {
+ Object obj = message.get(MAX_RESPONSE_CONTENT_LENGTH);
+ try {
+ maxResponseContentLength = Integer.parseInt(obj.toString());
+ } catch (NumberFormatException e) {
+ LOG.log(Level.WARNING, "INVALID_TIMEOUT_FORMAT", new Object[] {
+ MAX_RESPONSE_CONTENT_LENGTH, obj.toString()
+ });
+ }
+ }
+ if (maxResponseContentLength == null) {
+ maxResponseContentLength = SystemPropertyAction.getInteger(MAX_RESPONSE_CONTENT_LENGTH,
+ DEFAULT_MAX_RESPONSE_CONTENT_LENGTH);
+ }
Review comment:
removed
----------------------------------------------------------------
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
[GitHub] [cxf] vladikM commented on a change in pull request #684: [CXF-8317] Make netty transport HttpObjectAggregator.maxContentLength configurable
Posted by GitBox <gi...@apache.org>.
vladikM commented on a change in pull request #684:
URL: https://github.com/apache/cxf/pull/684#discussion_r457973409
##########
File path: core/src/main/java/org/apache/cxf/message/Message.java
##########
@@ -196,6 +196,12 @@
*/
String THREAD_SAFE_STAX_FACTORIES = Message.class.getName() + ".THREAD_SAFE_STAX_FACTORIES";
+
+ /**
+ * Integer property to specify how much bytes client must be able to handle.
+ */
+ String MAX_RESPONSE_CONTENT_LENGTH = Message.class.getName() + ".MAX_RESPONSE_CONTENT_LENGTH";
Review comment:
removed from Message
##########
File path: rt/transports/http/src/main/java/org/apache/cxf/transport/http/HTTPConduit.java
##########
@@ -652,6 +652,25 @@ protected static int determineConnectionTimeout(Message message,
return (int)ctimeout;
}
+ protected static int determineMaxResponseContentLength(Message message,
Review comment:
moved
----------------------------------------------------------------
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