David E Jones wrote:
> Are "on the same page" here, as it were? It sounds your conclusion is the
> same as my understanding, ie by the time it triggers these things either the
> commit is already done or the rollback is already done (or at least the first
> phase of them is done), and so it doesn't make sense to try to participate in
> the same transaction. Is that what you're saying?
>
> I'm wondering now what would happen if you did try to use the existing
> transaction... my guess is that whether it was a commit or rollback the
> transaction would not be in a state that would allow additional operations,
> so you'd probably get an invalid state exception. Just guessing though, I
> haven't actually tried it or looked at the docs to see if they say.
I've continued reading various doco about how XAResource is supposed
to work. prepare() is when the resource gets a chance to vote on
whether the transaction is committable or not. commit() and
rollback() are supposed to be used to actually do what the result of
the vote was. ServiceXaWrapper currently implements commit and
rollback, and based on my reading of specs and docs, it's ok to
actually run those async.
There are still problems with this class, however.
The runService method suspends the current transaction, and starts a
new one. However, that method is running in a brand new thread, that
doesn't have a transaction. So that's a useless thing to do.
Second, when async=true, and persist=true, the brand new transaction
in runService does nothing. The requested service will just end up
being serialized to JobSandbox, and will end up having it's own
transaction when it runs a little bit later.
Restating an earlier problem, in a slightly different way.
ServiceXaWrapper has 2 main knobs, async and persist. When both are
true, then ServiceXaWrapper has problems. Async is always true, due
to GenericAbstractDispatcher hard-coding it thusly. And all existing
calls that register a new ServiceXaWrapper, have persist set to true.
Also, based on my continued reading and thinking, the original problem
that caused me to investigate this code, is a pure bug in the original
code. The checkout process registers a rollback service, to undo
stuff when the transaction rolls back. The spec says that the
rollback will have already occurred when the XAResource rollback event
happens. So, the previous rollback service is very broken. The data
it is trying to read is gone, no longer available. If it needs to
interface to an external payment processor, then it will have to do it
in a very different way.
>
> -David
>
>
> On Apr 16, 2010, at 4:21 PM, Adam Heath wrote:
>
>> David E Jones wrote:
>>> It sounds like everything is as it should be. Maybe you just had incorrect
>>> assumptions about how this was supposed to work? In other words, is it
>>> possible the problem isn't with the design? I don't know about the code, I
>>> won't comment on that.
>>>
>>> In any case, are you saying that if something is triggered by an already
>>> established commit or rollback that a service triggered by it should be
>>> able to modify that somehow? What would be the point of trying to work in
>>> the context of a transaction that is already in the process of being
>>> committed (I think this happens between phases 1 and 2 of a 2-phase commit,
>>> if I remember right) or rolled back? Wouldn't it be a bad idea to do
>>> anything with that transaction?
>>>
>>> As fun as writing is, it's amazing how much more productive reading can be.
>>
>>
>> // Example code
>> boolean wasBad = false;
>> TransactionUtil.begin()
>> try {
>> CheckOutHelper.createOrder()
>> wasBad = ServicEUtil.isError(CheckOutHelper.processPayment());
>> } catch (Throwable t) {
>> wasBad = true;
>> } finally {
>> if (wasBad) {
>> TransactionUtil.rollback();
>> } else {
>> TransactionUtil.commit();
>> }
>> }
>> // GenericAbstractDispatcher:
>> // shows that the async flag is hard-coded to true.
>> public void addRollbackService(String serviceName, Map<String, ?
>> extends Object> context, boolean persist) throws GenericServiceException {
>> ServiceXaWrapper xa = new
>> ServiceXaWrapper(this.getDispatchContext());
>> xa.setRollbackService(serviceName, context, true, persist);
>> try {
>> xa.enlist();
>> } catch (XAException e) {
>> Debug.logError(e, module);
>> throw new GenericServiceException(e.getMessage(), e);
>> }
>> }
>>
>>
>> processPayment registers a rollback service, to unapply any payments.
>> This registration sets the persist flag to true. The context map of
>> that registration mentions an OrderPaymentPreference that was created
>> during the transaction.
>>
>> GenericAbstractDispatcher, which is responsible for actually creating
>> a ServiceXaWrapper instance, will always make ServiceXaWrapper run the
>> registered service in async mode. This is a hard-coded boolean true.
>>
>> Since all registered commit/rollback services are always in async
>> mode, there are 2 paths that they could end up taking. If the service
>> is persisted, it'll be saved to JobSandbox, and fetched from the job
>> poller with a slight delay. Or, the service could be submitted to the
>> job poller immediately, and then still processed slightly later, in
>> the case of the thread pool being busy. In either event, the service
>> is always run async.
>>
>> In the given example, persist is true, and async is always
>> true(hard-coded), so the passed context will be serialized to disk.
>> When deserialized, it'll try to refresh a
>> value(OrderPaymentPreference) from the entity engine, but that fails,
>> as the original value got rolled back. Since the async service
>> failed, a retry is scheduled, again thru JobSandbox, and this failing
>> service continues on forever attempting to process something that will
>> never succeed.
>>
>> With the rollback/commit event calls in ServiceXaWrapper creating
>> temporary new threads, there is no chance at all for them to see any
>> data that was altered by the parent transaction in the parent thread.
>> Plus, since the brand new child thread suspends the existing
>> transaction and creates a new one, again, there's no way for the
>> rollback/commit service to see any of the parent's data.
>>
>> Are the rollback/comment callbacks in ServiceXaWrapper called before
>> or after the transaction has been rolled back or committed?
>>
>> Ok, I'm answering my own question. The JTA spec says that enlisted
>> resources are run inside the parent transaction. This means that they
>> are allowed to see any data that was manipulated by the parent
>> transaction. However, in the ServiceXaWrapper case, this is not
>> happening. First, the internal runService helper method is in a
>> separate child thread. Secondly, the child thread creates brand new
>> transaction anyways, so the called service has no chance of seeing any
>> of it's parents data.
>>
>> All this is fine in the case of commit services, as the data they are
>> looking at is actually already committed to the database.
>>
>> However, rollback services have no chance at all to see any of their
>> parent data, because they don't run in the same transaction, and the
>> database no longer has any of the data.
>>
>>> On Apr 16, 2010, at 11:51 AM, Adam Heath wrote:
>>>
>>>> So, the service engine has this way cool feature, the ability to call
>>>> a service when the current transaction is rolled back, or committed.
>>>> However, in reality, it actually is completely and uterly broken.
>>>>
>>>> Both of these methods allow for persisting the job to the JobSandbox.
>>>> That in itself is not a problem.
>>>>
>>>> What is a problem, is that GenericAbstractDispatcher will *always*
>>>> make any such registered service an async service. There is no way to
>>>> run a sync service. Remember this point, I'll come back to it in a bit.
>>>>
>>>> Once the ServiceXaWrapper is registered with the transaction, and the
>>>> actual commit/rollback phase happens, it'll end up calling an internal
>>>> runService helper method. However, this helper method is run inside a
>>>> brand new, temporary thread. This effectively makes *all* registered
>>>> services async. That's just broken.
>>>>
>>>> One thing I'm not familiar with, is if a thread that spawns a new one
>>>> automatically inherits the parent thread's transaction. However, no
>>>> matter what that scenario actually is, when runService is running in
>>>> its own separate thread, it suspends the parent transaction. This
>>>> means it's in its very own separate transaction. So that the service
>>>> actually being run will *not* see anything that was in the original
>>>> transaction upon which it was registered.
>>>>
>>>> Now comes the time to actually run the real service. Since the
>>>> service is always an async service, it'll be serialized to JobSandbox,
>>>> and run a very short time later. Not immediately.
>>>>
>>>> Back in the original, parent thread, service, and transaction, the
>>>> transaction is committed or rolled back.
>>>>
>>>> The aforementioned async service(s) are now run. If the transaction
>>>> was rolled back, and they try to manipulate data, the original data
>>>> will no longer exist. So, the service will fail, and the JobPoller
>>>> will schedule a retry of the service. But, the service never has a
>>>> chance of succeeding, so this will repeating ad infinitum.
>>>>
>>>> I was able to track all this down, because we had
>>>> placeOrder/processPayment run inside a transaction, that was then
>>>> aborted if the payment was declined. We then started getting spammed
>>>> in the log with failed sandbox entries.
>>>>
>>>> Does anyone else agree with my reading of the code? Just looking at
>>>> the source I was able to discover these very poorly implemented designs.
>>>>
>>>>
>>>> Does anyone else agree with my reading here?
>
