ServiceXaWrapper is very very stupid and broken

[Adam Heath-2]

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?

[Scott Gray-2]

Your style of communication leaves a lot to be desired.

Regards
Scott

On 17/04/2010, at 4:51 AM, Adam Heath wrote:

>  Just looking at the source I was able to discover these very poorly implemented designs.
>
>
> Does anyone else agree with my reading here?

[Adam Heath-2]

Scott Gray wrote:
> Your style of communication leaves a lot to be desired.

And your point it?  Have you realized how poorly implemented this
class is?  That all registered services are always async.  That
rollbacks have no chance at all of working?  Such critical bugs not
being discovered in such low-level code make me very very worried.

[David E. Jones-2]

I dunno Scott, now that I've given up trying to discuss things with Adam I find his communications with others to be very entertaining... so for me there isn't much more to be desired... ;) (okay, sarcastic mode off).

-David


On Apr 16, 2010, at 3:26 PM, Scott Gray wrote:

> Your style of communication leaves a lot to be desired.
>
> Regards
> Scott
>
> On 17/04/2010, at 4:51 AM, Adam Heath wrote:
>
>> Just looking at the source I was able to discover these very poorly implemented designs.
>>
>>
>> Does anyone else agree with my reading here?
>

[David E. Jones-2]

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.

-David


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?

[Scott Gray-2]

On 17/04/2010, at 8:34 AM, Adam Heath wrote:

> Scott Gray wrote:
>> Your style of communication leaves a lot to be desired.
>
> And your point it?  Have you realized how poorly implemented this
> class is?  That all registered services are always async.  That
> rollbacks have no chance at all of working?  Such critical bugs not
> being discovered in such low-level code make me very very worried.

My point is that if you want people to respond to your messages then you should focus on the problem and possible solutions instead of using terms like "very very stupid" and "very poorly implemented designs".  I don't know about anyone else but when you communicate in this manner I personally have interest in collaborating with you.

Regards
Scott

[David E. Jones-2]

On Apr 16, 2010, at 3:51 PM, Scott Gray wrote:

> On 17/04/2010, at 8:34 AM, Adam Heath wrote:
>
>> Scott Gray wrote:
>>> Your style of communication leaves a lot to be desired.
>>
>> And your point it?  Have you realized how poorly implemented this
>> class is?  That all registered services are always async.  That
>> rollbacks have no chance at all of working?  Such critical bugs not
>> being discovered in such low-level code make me very very worried.
>
> My point is that if you want people to respond to your messages then you should focus on the problem and possible solutions instead of using terms like "very very stupid" and "very poorly implemented designs".  I don't know about anyone else but when you communicate in this manner I personally have interest in collaborating with you.

There is also a significant disconnect in Adam's email that fails to distinguish between designs and implementations. Most of the email talks about issues with the design, the whole implementation stuff is just thrown in there without details. I won't even get into the distinction between requirements and designs, but actually my guess is that is where Adam's frustration really is, his requirements are different than the ones this design was meant to meet, but failing to recognize that issue it just looks like a bad design and/or a bad implementation.

This leaves the reader wondering... what is the issue here? What is it you're trying to do that you can't? What is the proposed solution or change?

-David

[Adam Heath-2]

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?
>

[Adam Heath-2]

David E Jones wrote:

> On Apr 16, 2010, at 3:51 PM, Scott Gray wrote:
>
>> On 17/04/2010, at 8:34 AM, Adam Heath wrote:
>>
>>> Scott Gray wrote:
>>>> Your style of communication leaves a lot to be desired.
>>> And your point it?  Have you realized how poorly implemented this
>>> class is?  That all registered services are always async.  That
>>> rollbacks have no chance at all of working?  Such critical bugs not
>>> being discovered in such low-level code make me very very worried.
>> My point is that if you want people to respond to your messages then you should focus on the problem and possible solutions instead of using terms like "very very stupid" and "very poorly implemented designs".  I don't know about anyone else but when you communicate in this manner I personally have interest in collaborating with you.
>
> There is also a significant disconnect in Adam's email that fails to distinguish between designs and implementations. Most of the email talks about issues with the design, the whole implementation stuff is just thrown in there without details. I won't even get into the distinction between requirements and designs, but actually my guess is that is where Adam's frustration really is, his requirements are different than the ones this design was meant to meet, but failing to recognize that issue it just looks like a bad design and/or a bad implementation.
>
> This leaves the reader wondering... what is the issue here? What is it you're trying to do that you can't? What is the proposed solution or change?

I don't ask people to fix things, unless I actually ask them.  I was
trying to describe my problem, to see if someone else understands the
code the same way that I do.  If so, I can fix it myself.

If my understanding is wrong, fine.  Say so.  If you don't understand
what I am talking about, say that too, and I'll try again.  But just
saying your wrong, without telling me why, won't help the situation
either(not that this actually happened this time).

[Ruth Hoffman-2]

Hi David:
Not being as technically versed as most list members, I've got a
question that may seem obvious to you, but I was hoping you could clarify:

David E Jones wrote:

> On Apr 16, 2010, at 3:51 PM, Scott Gray wrote:
>
>  
>> On 17/04/2010, at 8:34 AM, Adam Heath wrote:
>>
>>    
>>> Scott Gray wrote:
>>>      
>>>> Your style of communication leaves a lot to be desired.
>>>>        
>>> And your point it?  Have you realized how poorly implemented this
>>> class is?  That all registered services are always async.  That
>>> rollbacks have no chance at all of working?  Such critical bugs not
>>>      

In what context does this statement make sense: "That rollbacks have no
chance at all of working?". Design or implementation? I'm confused
because, don't rollbacks work now? Or maybe, didn't they work at some
point? Or, is this a very specific situation that is implied by this post?

>>> being discovered in such low-level code make me very very worried.
>>>      
>> My point is that if you want people to respond to your messages then you should focus on the problem and possible solutions instead of using terms like "very very stupid" and "very poorly implemented designs".  I don't know about anyone else but when you communicate in this manner I personally have interest in collaborating with you.
>>    
>
> There is also a significant disconnect in Adam's email that fails to distinguish between designs and implementations. Most of the email talks about issues with the design, the whole implementation stuff is just thrown in there without details. I won't even get into the distinction between requirements and designs, but actually my guess is that is where Adam's frustration really is, his requirements are different than the ones this design was meant to meet, but failing to recognize that issue it just looks like a bad design and/or a bad implementation.
>
> This leaves the reader wondering... what is the issue here? What is it you're trying to do that you can't? What is the proposed solution or change?
>
> -David
>
>
>  

[Adam Heath-2]

Ruth Hoffman wrote:
> Hi David:
> Not being as technically versed as most list members, I've got a
> question that may seem obvious to you, but I was hoping you could clarify:

Your question is to me, not David.

>
> David E Jones wrote:
>> On Apr 16, 2010, at 3:51 PM, Scott Gray wrote:
>>
>>  
>>> On 17/04/2010, at 8:34 AM, Adam Heath wrote:
>>>
>>>    
>>>> Scott Gray wrote:
>>>>      
>>>>> Your style of communication leaves a lot to be desired.
>>>>>        
>>>> And your point it?  Have you realized how poorly implemented this
>>>> class is?  That all registered services are always async.  That
>>>> rollbacks have no chance at all of working?  Such critical bugs not
>>>>      
> In what context does this statement make sense: "That rollbacks have no
> chance at all of working?". Design or implementation? I'm confused
> because, don't rollbacks work now? Or maybe, didn't they work at some
> point? Or, is this a very specific situation that is implied by this post?

Rollback services can't work correctly.
dispatcher.addRollbackService(), specifically.

>>>> being discovered in such low-level code make me very very worried.
>>>>      
>>> My point is that if you want people to respond to your messages then
>>> you should focus on the problem and possible solutions instead of
>>> using terms like "very very stupid" and "very poorly implemented
>>> designs".  I don't know about anyone else but when you communicate in
>>> this manner I personally have interest in collaborating with you.
>>>    
>>
>> There is also a significant disconnect in Adam's email that fails to
>> distinguish between designs and implementations. Most of the email
>> talks about issues with the design, the whole implementation stuff is
>> just thrown in there without details. I won't even get into the
>> distinction between requirements and designs, but actually my guess is
>> that is where Adam's frustration really is, his requirements are
>> different than the ones this design was meant to meet, but failing to
>> recognize that issue it just looks like a bad design and/or a bad
>> implementation.
>>
>> This leaves the reader wondering... what is the issue here? What is it
>> you're trying to do that you can't? What is the proposed solution or
>> change?
>>
>> -David
>>
>>
>>  

[Ruth Hoffman-2]

Ok, sorry Adam.
Thanks

Adam Heath wrote:

> Ruth Hoffman wrote:
>  
>> Hi David:
>> Not being as technically versed as most list members, I've got a
>> question that may seem obvious to you, but I was hoping you could clarify:
>>    
>
> Your question is to me, not David.
>
>  
>> David E Jones wrote:
>>    
>>> On Apr 16, 2010, at 3:51 PM, Scott Gray wrote:
>>>
>>>  
>>>      
>>>> On 17/04/2010, at 8:34 AM, Adam Heath wrote:
>>>>
>>>>    
>>>>        
>>>>> Scott Gray wrote:
>>>>>      
>>>>>          
>>>>>> Your style of communication leaves a lot to be desired.
>>>>>>        
>>>>>>            
>>>>> And your point it?  Have you realized how poorly implemented this
>>>>> class is?  That all registered services are always async.  That
>>>>> rollbacks have no chance at all of working?  Such critical bugs not
>>>>>      
>>>>>          
>> In what context does this statement make sense: "That rollbacks have no
>> chance at all of working?". Design or implementation? I'm confused
>> because, don't rollbacks work now? Or maybe, didn't they work at some
>> point? Or, is this a very specific situation that is implied by this post?
>>    
>
> Rollback services can't work correctly.
> dispatcher.addRollbackService(), specifically.
>
>  
>>>>> being discovered in such low-level code make me very very worried.
>>>>>      
>>>>>          
>>>> My point is that if you want people to respond to your messages then
>>>> you should focus on the problem and possible solutions instead of
>>>> using terms like "very very stupid" and "very poorly implemented
>>>> designs".  I don't know about anyone else but when you communicate in
>>>> this manner I personally have interest in collaborating with you.
>>>>    
>>>>        
>>> There is also a significant disconnect in Adam's email that fails to
>>> distinguish between designs and implementations. Most of the email
>>> talks about issues with the design, the whole implementation stuff is
>>> just thrown in there without details. I won't even get into the
>>> distinction between requirements and designs, but actually my guess is
>>> that is where Adam's frustration really is, his requirements are
>>> different than the ones this design was meant to meet, but failing to
>>> recognize that issue it just looks like a bad design and/or a bad
>>> implementation.
>>>
>>> This leaves the reader wondering... what is the issue here? What is it
>>> you're trying to do that you can't? What is the proposed solution or
>>> change?
>>>
>>> -David
>>>
>>>
>>>  
>>>      
>
>
>  

[Adam Heath-2]

Replying to one of my own mails...

So, people took offense at the subject of the email I wrote.  That's
the only thing I can see wrong, and what people are complaining about.
 Ok, I can see that people may be offended by that.  So what?  Here's why.

If people would take the time to look at the possibility that I may
have found something broken, and it *does* turn out to be true, then
woo!  We have identified a bug, and we can go about getting it fixed.
 That's a good thing.  The problem wasn't known before now, but now it
is, so one unknown bug is gone.

Or, the other possibility is that I am wrong, and then I'm just an idiot.

Wouldn't it be better, to spend time reading the actual mail, reading
the actual code, and seeing if I am right or wrong?  Instead, what has
happened, is that people haven't even looked at the code in question;
 the immediate response is to attack me for even suggesting that a
problem exists.

When I sent my original email, I had spent time debugging and reading
code.  Instead of acknowledging that useful endeavor, people go on the
offensive.  Are we as a community not capable of taking criticism at
all?  Even when bugs actually *do* exist?

Note, I was not criticizing any particular person.  This code is very
complex, with a lot of moving parts.  Some of those parts may have
been added over a period of years(altho most are from before the
apache era days), so there may be a whole series of circumstances that
cause this issue I have to occur.

ps: If anyone responds to this mail saying that I am being compative
in *this* mail, then they are fucking around, wasting time.  I'm sick
of such people reading *way* to much into these things, and reading
them unlike any normal sane person would understand them.  This is a
repeat problem on this list with some people, over and over not
talking about problems as they are discovered, and instead just
attacking the deliver.

pps: If you haven't figured it out from my first ps:, yes, I'm pissed.

ppps: Actually, not really.  Just upset that others have converted
this very simple thread, about *fixing* a problem, into on attack on
my character.

[David E. Jones-2]

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.

-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?
>>
>

[Ean Schuessler]

I think the GWT gang sums it up pretty well in their "Please Be Friendly" section...

http://code.google.com/webtoolkit/makinggwtbetter.html#befriendly 

As they say, "There's never a reason to be antagonistic or dismissive toward anyone who is sincerely trying to contribute to a discussion" Now I do see your point that people could just lighten up a little bit and make some mental separation between saying "this code is very, very stupid" and "you, some actual person, is very, very stupid". I've seen code that's very stupid in plenty of projects and, heck, I've even written some code that was very stupid. We should be able to identify that a piece of code is stupid without walking on eggshells around the topic.

This doesn't waive your responsibility to keep things friendly. It doesn't add to the discussion to use overly colorful language or curse words that will obvious inflame people's emotions. The first rule you should keep in mind is that no-one *has* to work with you here. Don't be unfriendly, it doesn't help you or the project and you will create distance between yourself and your potential collaborators.

----- "Adam Heath" wrote:

> Replying to one of my own mails...
> So, people took offense at the subject of the email I wrote. That's
> the only thing I can see wrong, and what people are complaining about.
> Ok, I can see that people may be offended by that. So what? Here's why.
> If people would take the time to look at the possibility that I may
> have found something broken, and it *does* turn out to be true, then
> woo! We have identified a bug, and we can go about getting it fixed.
> That's a good thing. The problem wasn't known before now, but now it
> is, so one unknown bug is gone.
> Or, the other possibility is that I am wrong, and then I'm just an idiot.
> Wouldn't it be better, to spend time reading the actual mail, reading
> the actual code, and seeing if I am right or wrong? Instead, what has
> happened, is that people haven't even looked at the code in question;
> the immediate response is to attack me for even suggesting that a
> problem exists.
> When I sent my original email, I had spent time debugging and reading
> code. Instead of acknowledging that useful endeavor, people go on the
> offensive. Are we as a community not capable of taking criticism at
> all? Even when bugs actually *do* exist?
> Note, I was not criticizing any particular person. This code is very
> complex, with a lot of moving parts. Some of those parts may have
> been added over a period of years(altho most are from before the
> apache era days), so there may be a whole series of circumstances that
> cause this issue I have to occur.
> ps: If anyone responds to this mail saying that I am being compative
> in *this* mail, then they are fucking around, wasting time. I'm sick
> of such people reading *way* to much into these things, and reading
> them unlike any normal sane person would understand them. This is a
> repeat problem on this list with some people, over and over not
> talking about problems as they are discovered, and instead just
> attacking the deliver.
> pps: If you haven't figured it out from my first ps:, yes, I'm pissed.
> ppps: Actually, not really. Just upset that others have converted
> this very simple thread, about *fixing* a problem, into on attack on
> my character.

--
Ean Schuessler, CTO Brainfood.com
[hidden email] - http://www.brainfood.com - 214-720-0700 x 315

[Adrian Crum-2]

I would like to add one thing to Ean's excellent comments: Times are tough right now and everyone is under varying levels of stress. We might not respond the way we normally do because of the unusual circumstances forced upon us (I know I find myself reacting in ways I wouldn't do normally).

Maybe we can all pause, take a deep breath, and think about that. Then let's try to be patient with each other - as we all work our way though these tough times.

-Adrian


--- On Sat, 4/17/10, Ean Schuessler <[hidden email]> wrote:

> From: Ean Schuessler <[hidden email]>
> Subject: Re: ServiceXaWrapper is very very stupid and broken
> To: [hidden email]
> Cc: [hidden email]
> Date: Saturday, April 17, 2010, 10:17 PM
> I think the GWT gang sums it up
> pretty well in their "Please Be Friendly" section...
>
> http://code.google.com/webtoolkit/makinggwtbetter.html#befriendly
>
>
> As they say, "There's never a reason to be antagonistic or
> dismissive toward anyone who is sincerely trying to
> contribute to a discussion" Now I do see your point that
> people could just lighten up a little bit and make some
> mental separation between saying "this code is very, very
> stupid" and "you, some actual person, is very, very stupid".
> I've seen code that's very stupid in plenty of projects and,
> heck, I've even written some code that was very stupid. We
> should be able to identify that a piece of code is stupid
> without walking on eggshells around the topic.
>
> This doesn't waive your responsibility to keep things
> friendly. It doesn't add to the discussion to use overly
> colorful language or curse words that will obvious inflame
> people's emotions. The first rule you should keep in mind is
> that no-one *has* to work with you here. Don't be
> unfriendly, it doesn't help you or the project and you will
> create distance between yourself and your potential
> collaborators.
>
> ----- "Adam Heath" wrote:
> > Replying to one of my own mails...
> > So, people took offense at the subject of the email I
> wrote. That's
> > the only thing I can see wrong, and what people are
> complaining about.
> > Ok, I can see that people may be offended by that. So
> what? Here's why.
> > If people would take the time to look at the
> possibility that I may
> > have found something broken, and it *does* turn out to
> be true, then
> > woo! We have identified a bug, and we can go about
> getting it fixed.
> > That's a good thing. The problem wasn't known before
> now, but now it
> > is, so one unknown bug is gone.
> > Or, the other possibility is that I am wrong, and then
> I'm just an idiot.
> > Wouldn't it be better, to spend time reading the
> actual mail, reading
> > the actual code, and seeing if I am right or wrong?
> Instead, what has
> > happened, is that people haven't even looked at the
> code in question;
> > the immediate response is to attack me for even
> suggesting that a
> > problem exists.
> > When I sent my original email, I had spent time
> debugging and reading
> > code. Instead of acknowledging that useful endeavor,
> people go on the
> > offensive. Are we as a community not capable of taking
> criticism at
> > all? Even when bugs actually *do* exist?
> > Note, I was not criticizing any particular person.
> This code is very
> > complex, with a lot of moving parts. Some of those
> parts may have
> > been added over a period of years(altho most are from
> before the
> > apache era days), so there may be a whole series of
> circumstances that
> > cause this issue I have to occur.
> > ps: If anyone responds to this mail saying that I am
> being compative
> > in *this* mail, then they are fucking around, wasting
> time. I'm sick
> > of such people reading *way* to much into these
> things, and reading
> > them unlike any normal sane person would understand
> them. This is a
> > repeat problem on this list with some people, over and
> over not
> > talking about problems as they are discovered, and
> instead just
> > attacking the deliver.
> > pps: If you haven't figured it out from my first ps:,
> yes, I'm pissed.
> > ppps: Actually, not really. Just upset that others
> have converted
> > this very simple thread, about *fixing* a problem,
> into on attack on
> > my character.
>
> --
> Ean Schuessler, CTO Brainfood.com
> [hidden email]
> - http://www.brainfood.com - 214-720-0700 x 315
>
>
>

     

[Jacopo Cappellato-4]

I don't like this style too. And even if there are people in this list that seem to push strong in order to establish a new trend in the community interaction (less respect, impersonal communication, strong wording, harsh comments etc...) I still don't give up and accept this as the natural evolution of the project.
In fact, when an email starts like this I usually stop reading it and do something else.

Jacopo

On Apr 16, 2010, at 10:26 PM, Scott Gray wrote:

> Your style of communication leaves a lot to be desired.
>
> Regards
> Scott
>
> On 17/04/2010, at 4:51 AM, Adam Heath wrote:
>
>> Just looking at the source I was able to discover these very poorly implemented designs.
>>
>>
>> Does anyone else agree with my reading here?
>

[Adam Heath-2]

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?
>

[Adam Heath-2]

Ean Schuessler wrote:
> I think the GWT gang sums it up pretty well in their "Please Be Friendly" section...
>
> http://code.google.com/webtoolkit/makinggwtbetter.html#befriendly 
>
> As they say, "There's never a reason to be antagonistic or dismissive toward anyone who is sincerely trying to contribute to a discussion" Now I do see your point that people could just lighten up a little bit and make some mental separation between saying "this code is very, very stupid" and "you, some actual person, is very, very stupid". I've seen code that's very stupid in plenty of projects and, heck, I've even written some code that was very stupid. We should be able to identify that a piece of code is stupid without walking on eggshells around the topic.

Is this directed solely at me?  Or at others too?  As I have continued
to say, the very first email I sent I don't think was that
antagonistic.  And if it was(it very well could be interpreted that
way by some), then that still doesn't excuse others from not also
being friendly, as you are suggesting.

Consider this.  How often have any of us been doing some code reading,
some documentation reading, and then let out a little bit of
frustration, a little saying, something a little bit emotional.  And
then the people who are around you get all worried that something is
very bad, and come over to see what is going on.  When in actuality,
its completely innocuous.  Especially considering that as you continue
reading, and investigating, you find out your initial outburst was
unwarranted.

That's what my original email was.  A cry of frustration, and a dump
of the debug work I had done up to that point.  I was just looking for
a little bit of confirmation, or a pointer in a better direction.
Instead, people over-reacted on the outburst of frustration.

I've since kept reading and thinking, and ServiceXaWrapper by itself
is not all that bad.  But the existing users of it are currently doing
it in an incorrect manner, and it's those uses that really have to be
fixed.  Unfortunately, I had to discover this all on my own.

> This doesn't waive your responsibility to keep things friendly. It doesn't add to the discussion to use overly colorful language or curse words that will obvious inflame people's emotions. The first rule you should keep in mind is that no-one *has* to work with you here. Don't be unfriendly, it doesn't help you or the project and you will create distance between yourself and your potential collaborators.
>
> ----- "Adam Heath" wrote:
>> Replying to one of my own mails...
>> So, people took offense at the subject of the email I wrote. That's
>> the only thing I can see wrong, and what people are complaining about.
>> Ok, I can see that people may be offended by that. So what? Here's why.
>> If people would take the time to look at the possibility that I may
>> have found something broken, and it *does* turn out to be true, then
>> woo! We have identified a bug, and we can go about getting it fixed.
>> That's a good thing. The problem wasn't known before now, but now it
>> is, so one unknown bug is gone.
>> Or, the other possibility is that I am wrong, and then I'm just an idiot.
>> Wouldn't it be better, to spend time reading the actual mail, reading
>> the actual code, and seeing if I am right or wrong? Instead, what has
>> happened, is that people haven't even looked at the code in question;
>> the immediate response is to attack me for even suggesting that a
>> problem exists.
>> When I sent my original email, I had spent time debugging and reading
>> code. Instead of acknowledging that useful endeavor, people go on the
>> offensive. Are we as a community not capable of taking criticism at
>> all? Even when bugs actually *do* exist?
>> Note, I was not criticizing any particular person. This code is very
>> complex, with a lot of moving parts. Some of those parts may have
>> been added over a period of years(altho most are from before the
>> apache era days), so there may be a whole series of circumstances that
>> cause this issue I have to occur.
>> ps: If anyone responds to this mail saying that I am being compative
>> in *this* mail, then they are fucking around, wasting time. I'm sick
>> of such people reading *way* to much into these things, and reading
>> them unlike any normal sane person would understand them. This is a
>> repeat problem on this list with some people, over and over not
>> talking about problems as they are discovered, and instead just
>> attacking the deliver.
>> pps: If you haven't figured it out from my first ps:, yes, I'm pissed.
>> ppps: Actually, not really. Just upset that others have converted
>> this very simple thread, about *fixing* a problem, into on attack on
>> my character.
>

[David E. Jones-2]

On Apr 18, 2010, at 3:14 PM, Adam Heath wrote:

> 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.

Weren't you saying somewhere that when a thread spawns a new thread that it shares the XA context?

> 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.

It does one thing: it saves the JobSandbox record to the database in the transaction.

-David

> 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?
>>
>