You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by michaelandrepearce <gi...@git.apache.org> on 2017/12/07 23:08:57 UTC
[GitHub] activemq-artemis pull request #1695: ARTEMIS-1545 Ensure JMS security except...
GitHub user michaelandrepearce opened a pull request:
https://github.com/apache/activemq-artemis/pull/1695
ARTEMIS-1545 Ensure JMS security exceptions occur if NON-PERSISTENT
Add test case to ensure exception behaviour on JMS MessageProducer send is the same, if message is sent persistently or non-persistently when using default settings.
Update default setting to ensure behaviour is the same, as per expectation when using JMS by default. (e.g. no setting overrides)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-1545
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1695.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1695
----
commit 4bf4c9ba02fd85f01fcb958ae5bf4e27cb4aefc1
Author: Michael André Pearce <mi...@me.com>
Date: 2017-12-07T23:07:40Z
ARTEMIS-1545 Ensure JMS security exceptions occur if NON-PERSISTENT
Add test case to ensure exception behaviour on JMS MessageProducer send is the same, if message is sent persistently or non-persistently when using default settings.
Update default setting to ensure behaviour is the same, as per expectation when using JMS by default. (e.g. no setting overrides)
----
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
On note of perf it should note that it doesn’t stop someone setting it to true, it just makes the behaviour by default (eg expectation) to be correct, which is most important especially with adopting / migrating.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@andytaylor the issue i see is that the defaults should err on the side of safety imo.
Having a client not throw any exception and continue blindly is dangerous (even if its miss-config). Just because something is like it is, doesn't always been it should be.
I get the performance thing, but then thats something someone should make a conscious decision to enable at the expense of exceptions.
Just a bit of background, we migrated a flow (it was using JMS api, so switch is just in the CF) from another vendor's broker to Artemis, and released to prod.
The broker did alert, but the operations team operating the broker saw it as a client issue. and as was a client issue they believed the client team would also get an error, have been alerted and would have resolved.
Because the client team had no error, it was not until quite a period later it was discovered and external reports had to alert us to the issue.
We re-tested the scenario with our other brokers, and they would have alerted client side so this is why this assumption is had.
If we look to other people migrating I'm fairly sure the other brokers we use, are quite well used, and as such other users would make similar assumptions and expectation that the client would still error by default, unless they toggled something for performance on purpose.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
I'm not compelled by the "expectations" argument because different users have different expectations. Users from one JMS implementation might have an expectation that non-persistent messages are sent synchronously, but any user coming from HornetQ, Artemis, or (more importantly, I think) ActiveMQ 5.x among others would have the opposite expectation. This behavior is not defined by the spec, and therefore should be well documented so users will be informed and can change their expectations (if necessary).
I think the best way forward would be to improve the SendAcknowledgementHandler feature in Artemis which serves as the basis for the JMS CompletionHandler implementation.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
At this point I don't even think the broker is even sending anything back. It looks to me like the client simply infers success based on no exceptions when pushing the message into the transport layer.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
THe last comment was re SendAcknowledgementHandler improving
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
That’s true if you use the JMS 2.0 async send with an on completion listener. But this is older 1.1 sync send method.
Note that also there is an issue with the async send also in artemis client currently as onException is never called only onAcknowledge as such cannot make use of that due to that bug/issue.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
Eg if that worked then we could make use of that here to get the exception without blocking send.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
So I think we need the core layer to handle back exceptions it’s callback only seems be success only, anyone got any pointers where to start and how we should look to get that so it gets errors back async to pass to the callback
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@michaelandrepearce ok.. but I wouldn't change these defaults
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
I agree with Justin, I think its a bad idea to change this. we should find a different solution if we want to propagate exceptions back to the client.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@tabish121 this isn’t what we found on testing our two other incumbent brokers. There is a toggle on one of them to make it not wait and no exceptions but you have to actively override the default
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@clebertsuconic i think the point here is to get the exception back on the SendAcknowledgementHandler async then at least a either you can set an ExceptionListener to log these out in JMS (for JMS 1.1. methods) , and also allow for if a CompletionListener (for JMS 2.0 methods) is set on exception it can be passed to the listener.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@andytaylor to propagate exceptions on send (with it being sync) it will have to block somewhere, and wait for the return packet, i don't see much other options. It be good if there is one you have in mind if you could share.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
I'm not sure we'd want to change the default here as it will have significant performance implications.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
How would you solve this issue then? On testing performance in our system (it’s quite high perf) this really didn’t have any impact (if anything it reduced latency)
---
[GitHub] activemq-artemis pull request #1695: ARTEMIS-1545 Ensure JMS security except...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1695
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@michaelandrepearce it depends on what other URI options you've set on the 5.x client, it will at times switch back to sync based on other configuration. For alternate reference the Qpid JMS AMQP 1.0 client will also send non-persistent messages async by default, errors would be reported to the connection exception listener for JMS 1.1 sends and to the CompletionListener for JMS 2.0 async sends as one would expect.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
I think expected JMS behaviour is more important.
Persistence is about if the message should be persistent during broker restart/failure for broker side perfomance. It shouldn't change the behaviour of the client in regards to exceptions.
Like wise as a user you would expect client side exception by default (spec or no-spec).
When checking other vendors (We checked out other two brokers) and also 5.x, exception on send to the broker is thrown back to the client with non-persistent. (e.g. no difference between persistent or not in these case)
I think this is particularly important for anyone migrating (and pertinent with regards to 5.X users moving) that they would still get JMS exceptions on send and not get any surprises that they don't get them.
FYI this is what we are doing, migrating from several different brokers onto Artemis.
On the note of performance (as you may be aware or not) we do care for performance (throughput and latency) in our brokers due to nature of the business, but expected behaviour (aka no shocks) is more important. On that note we actually did do some tests and found produce to consume latency reduced per message.
Like wise for those really wanting it and willing to sacrifice no client side exceptions they still can set this to false consciously, but the point is the default should be that users do expect them, and probably should get such surprises.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@clebertsuconic yup thats the idea, i think, instead of this PR (e.g. i would close it), we look to sort SendAcknowledgementHandler instead.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
Actually I was mostly referring to JMS 1.1 sends where many JMS client opt to send non-persistent messages async and indeed won't report an error back unless you've explicitly configured the client to do so.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@andytaylor are you able to give any pointers, if we look to the root of improving the SendAcknowledgementHandler like @jbertram suggests. You probably know the broker bits a bit better?
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by andytaylor <gi...@git.apache.org>.
Github user andytaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@michaelandrepearce I dont have one :), however I dont see a reason to change defaults that have around for such a long time.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
Eg out the box you expect JMS behaviour to be correct that exceptions are thrown in these cases and likewise if unable to send to the broker it throw me exception. Behaviour should be the same if persistent or not, the difference only in no persistence is about if the message should be persistent during broker restart/failure
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
We could get exceptions packets back to the client... and cache there so any further asynchronous calls would fail with an exception...
Danger of that is.. it's easy to make a test where it fails...
```java
producer.send(nonPersistent);
connectionOnSameProducer.createSession(somethingElse); <<< Exception: Cannot send message!!!
```
User would be... What???
that's why it was never implemented...
Only real chance to fix this is to make a check at the producer create somehow. but that won't fix anonymous producers.
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
@jbertram I think that’s where I got to also, just don’t know where to start... any advice or tips?
---
[GitHub] activemq-artemis issue #1695: ARTEMIS-1545 Ensure JMS security exceptions oc...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1695
<Copied from the JIRA since we apparently don't read those comments any more>
This doesn't actually strike me as being a bug on the send as many JMS provider will indeed send the message asynchronously and if any error is reported on send fail it'd be done via the JMS ExceptionListener on the Connection. It would be nice if the createProducer failed here if not authorized to write to the target destination but if using an anonymous producer than again this seems to operate as many other JMS clients would.
The standard practice here would be to use a transaction to create a sync point (i.e. the commit) where the broker would report the operation as failed if any of the sends inside that TX failed for some reason. Since the NON_PERSISTENT delivery mode is 'at-most-once' you shouldn't expect the same QoS behaviors on the client send (unless of course you can configure that which appears you could via the block option).
---