[#1714] async send completion listener

[jeanouii]

No description provided.

[cshannon]

This will need to be reviewed in detail, there were problems with the previous attempt to address this in #1364 with not meeting the spec

One of the biggest issues as commented here was this part of the spec:
https://jakarta.ee/specifications/messaging/3.1/jakarta-messaging-spec-3.1#use-of-the-completionlistener-by-the-jakarta-messaging-provider

7.3.8. Use of the CompletionListener by the Jakarta Messaging provider
A session will only invoke one CompletionListener callback method at a time. For a given MessageProducer or JMSContext, callbacks (both onCompletion and onException) will be performed in the same order as the corresponding calls to the asynchronous send method.

@jeanouii - I haven't reviewed this yet but does this PR address that part (7.3.8) of the spec? Have you reviewed the other parts of the spec to verify if this is compatible?

[jeanouii]

@cshannon Yes I took a look at the PR and previous discussions.

Yes it explicitely addresses

7.3.8. Use of the CompletionListener by the Jakarta Messaging provider
A session will only invoke one CompletionListener callback method at a time. For a given MessageProducer or
JMSContext, callbacks (both onCompletion and onException) will be performed in the same order as the corresponding calls to the asynchronous send method.

Some key points I should have written in the PR description.

1. One callback at a time: the session holds an Executors.newSingleThreadExecutor(). As discussed in the last PR, this goes against parallelism but guarantees the constraint is met. Also we still have our AsyncCallback that can be used to increase concurrency.
2. Preserve order: the send is synchronized on the client side with the sendMutex. And the mutex is held until future.get() completes, which guarantees the sequentiality. This was your proposal I just implemented.
3. Callback not in the sender thread: that's exactly why there is the single thread executor.

BTW by reviewing the PR again in order to verify the 3 points, I discovered a possible deadlock which I just fixed in the last commit. The build is still green and the TCK all pass.

Hope it helps

[gemmellr]

I gave the async send bits a skim (ignored all the stuf really from other PRs), and noticed a couple things.

I kinda stopped looking though once I realised it isnt really implementing true asynchronous send at all (more just doing sync sends just with the async API, giving increasing API compatibility) and so various things its clearly not doing are thus far less of an issue right now. Even with that being the case though, the usage restriction checks issue could be problematic for folks using multiple sessions/connections.

[jbertram]

@jeanouii, you said:

...This is under the cover synchronous. This is purely spec related, and was a clear design choice.

I expect users will assume that they're getting real async given that's the intention of the spec. If users aren't getting real async then this should be clearly called out in the documentation.

[cshannon]

2. Preserve order: the send is synchronized on the client side with the sendMutex. And the mutex is held until future.get() completes, which guarantees the sequentiality. This was your proposal I just implemented.

As long as the spec is met I (speaking for myself) think that is ok as I feel like that is better than not implementing it at all. Truly async is certainly much harder to implement correctly given the spec requirements, so I'm ok starting with synchronous to at least get the API implemented but others may not agree with me.

If going the sync route, we can document the behavior (including javadocs etc) so users understand it's only one message at a time. The good news is that if we are following the spec we can make a future update to be truly async without breaking anyone.

[jeanouii]

@jbertram Yes we can certainly make sure it's clear for the users. Agreed with @cshannon and this is really what this is for. Having a spec compliant implementation. We always improve the boiler plate under the API to support full async.

All great discussion and feedback.
Do we have a wiki to capture things like this?

On my work projects I usually have an docs/adr with md/adoc files in there to capture technical thoughts and choices.
I like to have them versioned along side with the project, but maybe we use a wiki or any other means?