WFLY-6215 Don't allow afterCompletion callbacks to happen when an inv…

[stuartwdouglas]

…ocation is in progress

[scottmarlow]

Not sure if it is relevant but should we comment that other SessionSynchronization callbacks will have run already in the background (tm reaper) thread?

[scottmarlow]

retest this please

[dmlloyd]

Another possibility is that the transaction was manually acquired through some user code or framework and committed this way. I think it's at least more consistent to track the transaction outcome and report it, even if it is very likely always to be false.

[ryanemerson]

With this solution it is possible for EJB invocations to be executed on a SFSB after the Tx has rolledback and afterCompletion has already been executed. Shouldn't we throw an EJBTransactionRolledbackException to the client in this situation?

[stuartwdouglas]

@ryanemerson I am not really sure what you mean? Can you elaborate on the scenario you are describing?

[ryanemerson]

@stuartwdouglas Consider the following:

userTransaction.setTransactionTimeout(5);
userTransaction.begin();
sfsb.method1();
sfsb.method2();
.... 

In your solution, if the userTransaction timesout during sfsb.method1(), then sfsb.afterCompletion(...) is called, but no exception is thrown to the client, therefore sfsb.method2() will be executed after sfsb.afterCompletion(...).

I don't believe this is a violation of the specification, but I am not sure it is desirable behaviour. Would it not be better for sfsb.method2() to fail-fast and throw an exception to the client?

[stuartwdouglas]

If CMP is in use method1() will throw an exception when the CMP interceptor attempts to commit the transaction. If BMP is in use then an exception will not be thrown until the user attempts to do something transnational, which I think is the correct behaviour, even though in some cases it may be counter intuitive. We can't throw exceptions unless the spec actually requires it.

[n1hility]

@stuartwdouglas did you have a response or update to Scott and David's comments? I am thinking this would be an ideal 10.1 PR to merge.

[bstansberry]

@stuartwdouglas ^^^

[scottmarlow]

I'm not sure if WFLY-4923 is relevant to this change but also see my comments on #8731, which has the sync.afterCompletion callback check if the application has yet seen the transaction end, which is a hint as to whether the application thread or background reaper thread, should perform the afterCompletion action.

[n1hility]

@stuartwdouglas ping ^

[stuartwdouglas]

Linux failure looks like an unrelated remoting deadlock

[n1hility]

Retest this please