You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/02/05 21:23:38 UTC
[GitHub] [activemq] mattrpav opened a new pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
mattrpav opened a new pull request #756:
URL: https://github.com/apache/activemq/pull/756
…ical with server-side
- Handle in the OpenWireFormat class
- Add unit tests to confirm
- Verify compression is accounted for
- Verify the ability to disable using wireFormat.verifyMaxFrameSize=false
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon edited a comment on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon edited a comment on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1030933657
@mattrpav - I reworked your test to add in the other transports and cases (testing out a mixture of client/server enabled and disabled). I pushed it here to a branch https://github.com/cshannon/activemq/tree/AMQ-8142-updated
It now generates a total of 64 tests , see what you think
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800209094
##########
File path: activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOTransport.java
##########
@@ -139,8 +139,10 @@ protected void serviceRead() {
nextFrameSize = inputBuffer.getInt() + 4;
if (wireFormat instanceof OpenWireFormat) {
- long maxFrameSize = ((OpenWireFormat)wireFormat).getMaxFrameSize();
- if (nextFrameSize > maxFrameSize) {
+ OpenWireFormat openWireFormat = (OpenWireFormat)wireFormat;
+ long maxFrameSize = openWireFormat.getMaxFrameSize();
+
+ if (openWireFormat.isMaxFrameSizeEnabled() && nextFrameSize > maxFrameSize) {
throw new IOException("Frame size of " + (nextFrameSize / (1024 * 1024)) + " MB larger than max allowed " + (maxFrameSize / (1024 * 1024)) + " MB");
Review comment:
Let's do that part in 5.17.x since it could (low change) break server-side log parsers
ref: https://issues.apache.org/jira/browse/AMQ-8476
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1031439173
I have a negotiation test that I will push separate so I will merge this now.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon merged pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon merged pull request #756:
URL: https://github.com/apache/activemq/pull/756
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1030933657
@mattrpav - I reworked your test to add in the other transports and cases (testing out a mixture of client/server enabled and disabled). I pushed it here to a branch https://github.com/cshannon/activemq/tree/AMQ-8142-updated
It now generates a total of 48 tests , see what you think
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1030702789
@mattrpav - Thanks for the quick PR, I should be able to take a look first thing Monday morning
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800240683
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,11 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (cause instanceof MaxFrameSizeExceededException) {
+ JMSException jmsException = new JMSException(cause.getMessage(), "41300");
+ jmsException.setLinkedException(cause);
Review comment:
Instead of using linked I would use initCause() as thats what the server side does. A server side error thrown will have a TransportDisposedIOException or SocketException set when calling e.getCause() so it would be consistent
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800240683
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,11 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (cause instanceof MaxFrameSizeExceededException) {
+ JMSException jmsException = new JMSException(cause.getMessage(), "41300");
+ jmsException.setLinkedException(cause);
Review comment:
Instead of using linked I would use initCause() as thats what the server side does. A server side error thrown will have a TransportDisposedIOException set when calling e.getCause() so it would be consistent
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800240683
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,11 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (cause instanceof MaxFrameSizeExceededException) {
+ JMSException jmsException = new JMSException(cause.getMessage(), "41300");
+ jmsException.setLinkedException(cause);
Review comment:
Instead of using linked I would use initCause() as thats what the server side does. A server side error thrown will have a TransportDisposedIOExceptionset when calling e.getCause() so it would be consistent
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800250958
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,11 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (cause instanceof MaxFrameSizeExceededException) {
+ JMSException jmsException = new JMSException(cause.getMessage(), "41300");
+ jmsException.setLinkedException(cause);
Review comment:
My commit does both now
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800206637
##########
File path: activemq-client/src/main/java/org/apache/activemq/MaxFrameSizeException.java
##########
@@ -0,0 +1,31 @@
+/**
+ * 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.
+ */
+package org.apache.activemq;
+
+import java.io.IOException;
+/**
+ * An exception thrown when max frame size is exceeded.
+ *
+ *
+ */
+public class MaxFrameSizeException extends IOException {
+ private static final long serialVersionUID = -7681404582227153308L;
+
+ public MaxFrameSizeException(String message) {
Review comment:
done.
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,9 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (MaxFrameSizeException.class.isAssignableFrom(cause.getClass())) {
Review comment:
done.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1030864498
@mattrpav - Overall this is a big improvement over the previous version using getSize(). I verified the sizing and the reported frame sizes when checked on the client side vs server are now quite close (not exact but it's close enough)
I see a few things to change:
1. There needs to be tests for all the transports. You need to also test the normal transports (TCP, SSL, etc) and not just NIO.
2. the nio and nio+ssl transports actually check the frame size in a different location on the server than the OpenWireFormat so the NIOSSLTransport and NIOTransport should be updated to also check the new flag. See: https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOSSLTransport.java#L338 and https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOTransport.java#L142
3. I would change the isAssignableFrom() to an instanceof check as it's slightly fast (see the inline comment)
4. You should write edge case tests. For example, we should test all 3 scenarios: frame size is enabled on both client/server, frame size is only enabled on the server and frame size is only enabled on the client.
5. You should write a test to verify that the framesize flag is not negotiated over open wire to prevent errors in the future or someone changing it. Ie make sure that if either side changes the flag the other side doesn't have it's flag changed.
6. Documentation needs to be added both to the code and to the website that makes it very clear that the flag won't be negotiated and only applies where it is set. https://activemq.apache.org/configuring-wire-formats
7. I think we can probably just rename the flag `verifyMaxFrameSizeEnabled `to just `maxFrameSizeEnabled` . Your comment for the PR also has the wrong name.
8. Lastly I don't think this should be added to 5.16.4 due to it being a change in behavior and a feature. Point releases should really just be about bug fixes and this definitely changes the expected behavior a bit. I think we should only put this into 5.17.0. The flag applies to client/server so we can't put it into 5.16.4 and by default turn it off easily so that's why I think just 5.17.0
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800207966
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,9 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (cause instanceof MaxFrameSizeExceededException) {
+ return new JMSException(cause.getMessage(), "41300");
Review comment:
I'd include the max frame size exception as the root cause so the root cause exists and is a type of IOException whether server or client
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon edited a comment on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon edited a comment on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1030864498
@mattrpav - Overall this is a big improvement over the previous version using getSize(). I verified the sizing and the reported frame sizes when checked on the client side vs server are now quite close (not exact but it's close enough)
I see a few things to change:
1. There needs to be tests for all the transports. You need to also test the normal transports (TCP, SSL, etc) and not just NIO.
2. the nio and nio+ssl transports actually check the frame size in a different location on the server than the OpenWireFormat so the NIOSSLTransport and NIOTransport should be updated to also check the new flag. See: https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOSSLTransport.java#L338 and https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOTransport.java#L142
3. I would change the isAssignableFrom() to an instanceof check as it's slightly fast (see the inline comment)
4. You should write edge case tests. For example, we should test all 3 scenarios: frame size is enabled on both client/server, frame size is only enabled on the server and frame size is only enabled on the client. If enabled on the client then we should make sure the connection stays open and if it's a server side only check then the connection should be closed.
5. You should write a test to verify that the framesize flag is not negotiated over open wire to prevent errors in the future or someone changing it. Ie make sure that if either side changes the flag the other side doesn't have it's flag changed.
6. Documentation needs to be added both to the code and to the website that makes it very clear that the flag won't be negotiated and only applies where it is set. https://activemq.apache.org/configuring-wire-formats
7. I think we can probably just rename the flag `verifyMaxFrameSizeEnabled `to just `maxFrameSizeEnabled` . Your comment for the PR also has the wrong name.
8. ~~Lastly I don't think this should be added to 5.16.4 due to it being a change in behavior and a feature. Point releases should really just be about bug fixes and this definitely changes the expected behavior a bit. I think we should only put this into 5.17.0. The flag applies to client/server so we can't put it into 5.16.4 and by default turn it off easily so that's why I think just 5.17.0~~ I changed my mind, I think it's ok in 5.16.4 and likely a nice improvement. The behavior doesn't actually change on the client side since a JMSException is thrown either way (and the cause is an IOException still whether it's server or client side check if you make the change to include the MaxFrameSizeException as the cause). So the client gets the same exception type so it isn't really a breaking change. And it's better as the server doesn't have to deal with it
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800250768
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,11 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (cause instanceof MaxFrameSizeExceededException) {
+ JMSException jmsException = new JMSException(cause.getMessage(), "41300");
+ jmsException.setLinkedException(cause);
Review comment:
Should we do both? A number of the JMSExceptionSupport hanlding is linking both in. only costs a pointer
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon edited a comment on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon edited a comment on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1030864498
@mattrpav - Overall this is a big improvement over the previous version using getSize(). I verified the sizing and the reported frame sizes when checked on the client side vs server are now quite close (not exact but it's close enough)
I see a few things to change:
1. There needs to be tests for all the transports. You need to also test the normal transports (TCP, SSL, etc) and not just NIO.
2. the nio and nio+ssl transports actually check the frame size in a different location on the server than the OpenWireFormat so the NIOSSLTransport and NIOTransport should be updated to also check the new flag. See: https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOSSLTransport.java#L338 and https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOTransport.java#L142
3. I would change the isAssignableFrom() to an instanceof check as it's slightly fast (see the inline comment)
4. You should write edge case tests. For example, we should test all 3 scenarios: frame size is enabled on both client/server, frame size is only enabled on the server and frame size is only enabled on the client. If enabled on the client then we should make sure the connection stays open and if it's a server side only check then the connection should be closed.
5. You should write a test to verify that the framesize flag is not negotiated over open wire to prevent errors in the future or someone changing it. Ie make sure that if either side changes the flag the other side doesn't have it's flag changed.
6. Documentation needs to be added both to the code and to the website that makes it very clear that the flag won't be negotiated and only applies where it is set. https://activemq.apache.org/configuring-wire-formats
7. I think we can probably just rename the flag `verifyMaxFrameSizeEnabled `to just `maxFrameSizeEnabled` . Your comment for the PR also has the wrong name.
8. Lastly I don't think this should be added to 5.16.4 due to it being a change in behavior and a feature. Point releases should really just be about bug fixes and this definitely changes the expected behavior a bit. I think we should only put this into 5.17.0. The flag applies to client/server so we can't put it into 5.16.4 and by default turn it off easily so that's why I think just 5.17.0
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800201760
##########
File path: activemq-client/src/main/java/org/apache/activemq/MaxFrameSizeException.java
##########
@@ -0,0 +1,31 @@
+/**
+ * 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.
+ */
+package org.apache.activemq;
+
+import java.io.IOException;
+/**
+ * An exception thrown when max frame size is exceeded.
+ *
+ *
+ */
+public class MaxFrameSizeException extends IOException {
+ private static final long serialVersionUID = -7681404582227153308L;
+
+ public MaxFrameSizeException(String message) {
Review comment:
I would make this a little more descriptive. Maybe MaxFrameSizeExceededException instead.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] mattrpav commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
mattrpav commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800211696
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,9 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (cause instanceof MaxFrameSizeExceededException) {
+ return new JMSException(cause.getMessage(), "41300");
Review comment:
done
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800208475
##########
File path: activemq-client/src/main/java/org/apache/activemq/transport/nio/NIOTransport.java
##########
@@ -139,8 +139,10 @@ protected void serviceRead() {
nextFrameSize = inputBuffer.getInt() + 4;
if (wireFormat instanceof OpenWireFormat) {
- long maxFrameSize = ((OpenWireFormat)wireFormat).getMaxFrameSize();
- if (nextFrameSize > maxFrameSize) {
+ OpenWireFormat openWireFormat = (OpenWireFormat)wireFormat;
+ long maxFrameSize = openWireFormat.getMaxFrameSize();
+
+ if (openWireFormat.isMaxFrameSizeEnabled() && nextFrameSize > maxFrameSize) {
throw new IOException("Frame size of " + (nextFrameSize / (1024 * 1024)) + " MB larger than max allowed " + (maxFrameSize / (1024 * 1024)) + " MB");
Review comment:
I'd use the createFrameSizeException() here
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on a change in pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on a change in pull request #756:
URL: https://github.com/apache/activemq/pull/756#discussion_r800201575
##########
File path: activemq-client/src/main/java/org/apache/activemq/util/JMSExceptionSupport.java
##########
@@ -61,6 +63,9 @@ public static JMSException create(Exception cause) {
if (cause instanceof JMSException) {
return (JMSException)cause;
}
+ if (MaxFrameSizeException.class.isAssignableFrom(cause.getClass())) {
Review comment:
I would use instanceof here instead. isAssignableFrom is a dynamic call vs compile time so the performance is slightly worse.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq] cshannon commented on pull request #756: [AMQ-8412] Update client-side maxFrameSize handling to be more symetr…
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #756:
URL: https://github.com/apache/activemq/pull/756#issuecomment-1030920174
@mattrpav - In progress changes looking good so far, thanks!
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org