Thanks Chris. I looked at the PR - looks good. I hope nobody relies on our specific sub-classes of JMSException...
Art On Tue, Jan 16, 2024 at 5:21 PM Christopher Shannon < christopher.l.shan...@gmail.com> wrote: > 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? > > > > > > > > > > > > > > > > > >
