Art, For your first point, existing clients will run into this but they already properly handle the ClassNotFoundException fine so the client never sees the error. The marshaller just converts to a generic Throwable exception when it can't find the type given on the classpath and then the ActiveMQ client sees that and converts that to a JMSException. So it doesn't cause a client error but the exception type isn't quite right. (And no the broker doesn't create the type). My update fixes things so the client can translate easily back to the correct type so that the more specific JMS exception type and not a generic JMSException is thrown by the client.
There is no breaking change so there is no need to opt in. This solution is fixing a bug when trying to convert exception types so there is no reason to allow turning it off. Without the change if a javax client talks to a jakarta broker (or vice versa) the exception type won't be mapped correctly and just be generic as I said. When we make things better in the future and use a new openwire version to handle things, this should be transparent and the client will still correctly use the right exception types by converting the newly returned error codes to the right exceptions so from the users stand point and client behavior there's no real change. I made all the changes and pushed up to the branches and tested things, you can see the commits linked to the Jira: https://issues.apache.org/jira/browse/AMQ-9418 Chris On Tue, Jan 16, 2024 at 5:40 PM Arthur Naseef <a...@amlinv.com> wrote: > Looks like the key points here are: > > 1. Existing clients will run into ClassNotFoundException when > unmarshalling an exception from a non-compatible broker. (Do broker's > ever > create an instance of the class specified by the client?) > 2. The proposed change will update clients to convert between javax.jms > and jakarta.jms package names > 3. In the future we can look to replace the existing approach with > something safer/simpler > > If so, I like that approach. One thing I can see that could be a concern > for users - going to the intermediate solution means that they will hit > another breaking change in the not-too-distant future when we get to (3). > I wonder if we could perhaps make it opt-in so users are pushed to > understand what they are getting into. > > Art > > > > On Tue, Jan 16, 2024 at 1:04 PM Christopher Shannon < > christopher.l.shan...@gmail.com> wrote: > > > I talked with Tim and Matt offline and will go with that approach, I have > > PRs open. If needed we can backport to some older clients too. > > > > On Tue, Jan 16, 2024 at 9:38 AM Christopher Shannon < > > christopher.l.shan...@gmail.com> wrote: > > > > > As background, last week it was discovered (thanks Justin) that there > was > > > an oversight with the Exception handling over OpenWire with the upgrade > > to > > > Jakarta apis in the 6.0.x broker. > > > https://issues.apache.org/jira/browse/AMQ-9418 > > > > > > The problem is that OpenWire has a response type to return an exception > > to > > > the client and this response type includes the actual throwable type > > > written as a string with the fully qualified package. This means that > if > > an > > > older 5.x client that is using the javax JMS apis receives back > something > > > like InvalidClientIDException from a 6.x broker the exception type > > received > > > is actually going to be "jakarta.jms.InvalidIDException" and then the > > > client can't unmarshall that since it's likely not on the classpath. > The > > > result is that if it converts it will get a ClassNotFoundException and > > just > > > convert to a generic exception type so this could be a breaking change > > for > > > clients that are trying to handle specific exceptions. Note that the > > > reverse is actually also true, if a new 6.x client connected to a 5.x > > > broker the same problem happens. > > > > > > I've been looking at ways to fix this broker side and it's not great. > We > > > would need to detect if the client wants jakarta or javax types (we can > > > kind of do this for newer clients because > > > https://issues.apache.org/jira/browse/AMQ-6379 has new clients set > their > > > actual client version from the manifest file in the jar) and then we > have > > > to pass that to the marshallers and signal in some way to convert. The > > > main problem is having to pass it to the marshaller means probably > having > > > to update the Openwire format to include the version discovered and > > having > > > complicated detection logic depending on if it is set or not as not > > > everything sets it. If we didn't want to do it at the marshaller level, > > > we'd have to include or repackage both versions of the Exceptions which > > is > > > also not ideal as having to include both javax and jakarta exception > > types > > > could cause mistakes elsewhere and maintaining isn't fun. > > > > > > I think a much simpler approach is to just let the receiver (usually > > > client) handle it. I think instead of trying to have the broker detect > > what > > > to send, it should be handled on the unmarshal side of things by the > > > receiver of the error command. The receiver (client or broker) knows if > > it > > > will need to convert so it makes things much simpler. Obviously this > > would > > > only work if we update the marshaller in the client code jar so my > > proposal > > > would be to just release a small update in the next version of 5.18.x > > > client and also 6.0.x clients to handle this. We can just document > saying > > > that if you need to have specific exception type translation and are > > using > > > a broker and client that are not both javax or jakarta then you need to > > > upgrade to a new client version for the translation. Otherwise they can > > > just get the generic Throwable back and will work. The main use case I > > > think is if clients are stuck on older versions of Java and can't > upgrade > > > yet when talking to a new broker or an older broker can't be upgraded > yet > > > but the clients can. > > > > > > Here is an example of fixing the 5.18.x client: > > > > > > https://github.com/cshannon/activemq/commit/b52bdd3f1cdac31e9aa6fed86e6737d376b8b5a4 > > > It's pretty simple, if we get back jakarta then convert to javax and > > done. > > > We would do the reverse in 6.x branch. > > > > > > Going forward as a permanent fix, we should just fix this entirely in > > > OpenWire v13. Including the Java exception types already caused a major > > CVE > > > issue and now it's causing issues here. I think for the next OpenWire > > > version we should create a new Exception type that uses an Error code > > just > > > like Artemis and other projects do and then the client can just take > the > > > code and convert it to whatever exception it wants. We could either > > include > > > the error handling change by itself in OpenWire v13 so we could even > > > upgrade older 5.x brokers with it or just include the new openwire > > > exception handling only for 6.x along with shared sub OpenWire changes. > > > > > > Thoughts? > > > > > > > > > > > >
