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? > > > > > > >
