[
https://issues.apache.org/jira/browse/AMQ-8412?focusedWorklogId=721670&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-721670
]
ASF GitHub Bot logged work on AMQ-8412:
---------------------------------------
Author: ASF GitHub Bot
Created on: 06/Feb/22 17:09
Start Date: 06/Feb/22 17:09
Worklog Time Spent: 10m
Work Description: 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-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 721670)
Time Spent: 2h 20m (was: 2h 10m)
> Return a well-formed response to clients when max message size is sent
> ----------------------------------------------------------------------
>
> Key: AMQ-8412
> URL: https://issues.apache.org/jira/browse/AMQ-8412
> Project: ActiveMQ
> Issue Type: New Feature
> Components: JMS client
> Reporter: Matt Pavlovich
> Assignee: Jean-Baptiste Onofré
> Priority: Major
> Fix For: 5.17.0, 5.16.4
>
> Time Spent: 2h 20m
> Remaining Estimate: 0h
>
> Currently, clients get an inconclusive exception when a message that is too
> large is sent. We should send a well-formed message, and then close.
> Options:
> 1. Change the current IOException to something else to fall within existing
> exception flow
> 2. Update current exception handling to send ExceptionResponse w/ the max
> message size message to the client before closing
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
