I don't think that's an issue with our transaction handling, it's simply a
problem that the cache isn't transaction aware.
If the cache were to be transaction aware it would need to implement the
XAResource interface or perhaps even the simpler Synchronization interface and
push cache entries to the global cache only upon commit or discard them on
rollback. I'm loathe to suggest XAResource because we don't implement it
properly in GenericXaResource/ServiceXaWrapper/DebugXaWrapper and it breaks
some transaction managers (Atomikos is the only one I've tried). I have a
strong feeling it could be implemented using the Synchronization interface
without too much trouble though.
Regards
Scott
On 26/08/2014, at 9:46 pm, Adrian Crum <adrian.c...@sandglass-software.com>
wrote:
> The concepts of "suspend" and "resume" are implemented by a ThreadLocal
> stack. A "suspend" pushes the current transaction on the stack, and a
> "resume" pops a transaction off the stack.
>
> If you read the Jira issue, I point out another problem with this clunky
> implementation - calling "commit" doesn't really commit the transaction. That
> is why we end up with invalid data in the entity cache - because developers
> are fooled into thinking the "commit" calls in Delegator code actually commit
> the data, but they don't. The transaction is committed by the first bit of
> code that began the transaction - either a request event or a service
> invocation.
>
> This is an arcane problem and it is difficult to describe, but I will try to
> diagram it:
>
> Request Event
> Service Dispatcher
> Begin Transaction (actual begin)
> Begin Service
> Some service logic
> Delegator calls "commit" - nothing happens
> Delegator puts uncommitted values in cache
> More service logic
> Delegator calls "commit" - nothing happens
> Delegator puts uncommitted values in cache
> End Service
> Commit Transaction (actual commit)
> Return service results to event handler
>
> If something goes wrong in the service and the transaction is rolled back,
> the uncommitted values in the cache are still there!
>
> You really have to spend time in Entity Engine code to fully appreciate how
> awful the transaction implementation really is.
>
> My approach keeps a Transaction reference in the Delegator. Instead of
> calling the fake "commit", the Delegator notifies the Transaction about
> changed values. The Transaction saves the changed values locally. After the
> transaction is committed, the Transaction instance copies the saved values to
> the cache.
>
> If you look at my previous code fragment, there will be no more "suspend" or
> "resume" - if you want a new transaction, you just get another instance and
> use it.
>
> Adrian Crum
> Sandglass Software
> www.sandglass-software.com
>
> On 8/26/2014 9:02 PM, Scott Gray wrote:
>> Okay so I guess I don't really understand what you're suggesting, or how it
>> really differs much from what we have now. It's also not clear what your
>> suggested API changes have to do with the ThreadLocal usages?
>>
>> Thanks
>> Scott
>>
>> On 26/08/2014, at 3:22 pm, Adrian Crum <adrian.c...@sandglass-software.com>
>> wrote:
>>
>>> Just use the Delegator factory.
>>>
>>> Adrian Crum
>>> Sandglass Software
>>> www.sandglass-software.com
>>>
>>> On 8/26/2014 2:43 PM, Scott Gray wrote:
>>>> Hi Adrian,
>>>>
>>>> I'll probably have plenty of questions, but the first that comes to mind
>>>> is: how would you use a delegator outside of a transaction with this
>>>> approach?
>>>>
>>>> Thanks
>>>> Scott
>>>>
>>>> On 25/08/2014, at 10:51 am, Adrian Crum
>>>> <adrian.c...@sandglass-software.com> wrote:
>>>>
>>>>> One persistent problem with the current Entity Engine implementation is
>>>>> the use of ThreadLocal variables in the Delegator and Transactions. Their
>>>>> use makes it difficult (and sometimes impossible) to fix Entity Engine
>>>>> bugs. They also make it impossible to multi-thread a Delegator instance.
>>>>>
>>>>> Here is what I have had percolating in my head the last few months:
>>>>>
>>>>> Transaction tx = TransactionFactory.newTransaction();
>>>>> Delegator delegator = tx.getDelegator("default");
>>>>> // Do stuff with delegator
>>>>> Transaction nestedTx = TransactionFactory.newTransaction();
>>>>> Delegator nestedDelegator = nestedTx.getDelegator("default");
>>>>> // Do stuff with nestedDelegator
>>>>> nestedTx.commit();
>>>>> tx.commit();
>>>>>
>>>>> A Delegator instance always references the transaction it is running in.
>>>>>
>>>>> The advantage to this approach is we gain the ability to hand off
>>>>> Delegator instances to other threads. Other threads can even
>>>>> commit/rollback a transaction:
>>>>>
>>>>> Transaction tx = delegator.getTransaction();
>>>>> tx.commit();
>>>>>
>>>>> After a commit, the Delegator instance is discarded. Any attempt to use
>>>>> it after a commit throws an exception (the same is true with the
>>>>> Transaction instance).
>>>>>
>>>>> Another problem is Delegator localization - which also uses ThreadLocal
>>>>> variables. We can localize Delegator instances like this:
>>>>>
>>>>> Transaction tx = TransactionFactory.newTransaction();
>>>>> Delegator delegator = tx.getDelegator("default", locale);
>>>>>
>>>>> Finally, the current implementation has a caching problem:
>>>>> https://issues.apache.org/jira/browse/OFBIZ-5534
>>>>>
>>>>> With the new design, the Delegator instance, Transaction instance, and
>>>>> entity cache are tightly coupled - so that problem is easy to solve.
>>>>>
>>>>> What do you think?
>>>>>
>>>>> --
>>>>> Adrian Crum
>>>>> Sandglass Software
>>>>> www.sandglass-software.com
>>>>
>>
