1 2 Previous Next 24 Replies Latest reply on Jun 2, 2005 10:52 AM by adrian.brock

    mt-issues with org.jboss.tm.TransactionLocal

    dimitris

      Although I started examining the TransactionLocal backends for thread-safety (i.e. TransactionLocalDelegateImpl + TransactionLocalSynchronization & TxManager + TransactionImpl), it seems to me that TransactionLocal itself needs to be more thread-safe because:

      1) there is no other way to guarantee consistency between get()/set() independent from the delegate implementation. ( get() is an 1+1 step process: getValue() and potential storeValue(), set() a 2 step process: containsValue() and storeValue() )

      2) no other way to guarantee initialValue() is executed exactly once.

      I'm looking forward the following changes:

      NOW

      ...
       public Object get(Transaction transaction)
       {
       if (transaction == null) return initialValue();
      
       Object value = getValue(transaction);
      
       // is we didn't get a value initalize this object with initialValue()
       if(value == null)
       {
       // get the initial value
       value = initialValue();
      
       // if value is null replace it with the null value standin
       if(value == null)
       {
       value = NULL_VALUE;
       }
      
       // store the value
       storeValue(transaction, value);
       }
      
       // if the value is the null standin return null
       if(value == NULL_VALUE)
       {
       return null;
       }
      
       // finall return the value
       return value;
       }
      ...
       public void set(Transaction transaction, Object value)
       {
       if (transaction == null) throw new IllegalStateException("there is no transaction");
       // If this transaction is unknown, register for synchroniztion callback,
       // and call initialValue to give subclasses a chance to do some
       // initialization.
       if(!containsValue(transaction))
       {
       initialValue();
       }
      
       // if value is null replace it with the null value standin
       if(value == null)
       {
       value = NULL_VALUE;
       }
      
       // finally store the value
       storeValue(transaction, value);
       }
      ...
      

      CHANGES
      ...
       public Object get(Transaction transaction)
       {
       // if a transaction is not yet associated with the thread,
       // return null to avoid calling initialValue() multiple times
       if (transaction == null)
       {
       return null;
       }
       // make the getValue() and potential storeValue()
       // atomic and avoid calling initialValue() twice
       synchronized (this)
       {
       Object value = getValue(transaction);
      
       // If we didn't get a value, get/set has not been called
       // so call initialValue() to give subclasses a chance to
       // do some initialization, and store the provided value
       if (value == null)
       {
       // get the initial value
       value = initialValue();
      
       // replace a null value with the NULL standin
       if (value == null)
       {
       value = NULL_VALUE;
       }
       // store the initial value
       storeValue(transaction, value);
       }
       // convert back the NULL standin to null, if needed
       if (value == NULL_VALUE)
       {
       value = null;
       }
       // return the stored value
       return value;
       }
       }...
       public void set(Transaction transaction, Object value)
       {
       if (transaction == null)
       {
       throw new IllegalStateException("there is no transaction");
       }
       // make the containsValue()/storeValue() atomic
       // and avoid calling initialValue() twice
       synchronized (this)
       {
       // If get/set has not been called, call initialValue() to
       // give subclasses a chance to do some initialization,
       // even if the initial value is not used.
       if (!containsValue(transaction))
       {
       initialValue();
       }
       // replace a null value with the NULL standin
       if (value == null)
       {
       value = NULL_VALUE;
       }
       // finally store the value
       storeValue(transaction, value);
       }
       }
      ...
      


      Does it make sense? It will lock the TransactionLocal instance during the 2 operations, but I don't see how to avoid this.

      Another change I'm seeing, is that currently get(Tx) does:
      ...
       public Object get(Transaction transaction)
       {
       if (transaction == null) return initialValue();
      ...
      

      when no tx is associated with the thread. But that means initialValue() can be executed many times, until the thread becomes associated with a tx.

      Instead of throwing an exception (which is what set(tx) does), I imagine it is better to just return null, thus avoiding the multiple calls to initialValue(), and not break existing implementations.

      Any views are welcome.

        • 1. Re: mt-issues with org.jboss.tm.TransactionLocal

          Yes, this code needs making threadsafe, but not in the way you have done it.

          This is exactly the type of code that leads to deadlocks:

          public void doSomething()
          {
           synchronized(this)
           {
           delegate.doSomething();
           }
          }
          


          Thread1 synchronized(somethingElse)
          Thread2 ThreadLocal.doSomething() -> synchronized(ThreadLocal)
          Thread1 ThreadLocal.doSomething() -> wait
          Thread2 delegate.doSomething() -> synchronized(somethingElse)

          DEADLOCK!!!!

          You should never do
          synchronized(something)
          {
           somethingElse.call();
          }
          


          Unless something and somethingElse have been designed with a protocol
          that orders their access.

          In the case of ThreadLocal this is NOT TRUE since we do not control
          initialValue() or even the "delegate" implementation.

          • 2. Re: mt-issues with org.jboss.tm.TransactionLocal
            bill.burke

            A few years ago, I rewrote TransactionLocal to avoid Java synchronization. Although not as bad as the global synchronization that was going on a few years ago, your changes reintroduce some level of global synchronization.

            I think the TransactionLocalDelegate implementation should be handling synchronization. That way, our implementation can limit synchronization to only within threads in the same transaction. This will cause most of the logic to move out of TransactionLocal into the delegate, but this is ok.

            public class TransactionImpl {
            
             Object getTransactionLocalValue(TransactionLocal tlocal)
             {
             synchronized(transactionLocalMap) {
             if (transactionLocalMap.containsKey(local) { return transactionLocalMap.get(local);
             Object initial = local.initialValue();
             if (value == null) value = NULL_VALUE;
             transactionLocalMap.put(local, value);
             return value;
             }
             }
            
             void putTransactionLocalValue(TransactionLocal tlocal, Object value)
             {
             synchronized(tlocal) {
             ...
             }
             }
            
             boolean containsTransactionLocal(TransactionLocal tlocal)
             {
             synchronized(tLocal) {
             ...
             }
             }
            }
            


            • 3. Re: mt-issues with org.jboss.tm.TransactionLocal
              bill.burke

               

              "adrian@jboss.org" wrote:

              In the case of ThreadLocal this is NOT TRUE since we do not control
              initialValue() or even the "delegate" implementation.


              Yes, we do control the delegate implementation. WE MUST TAKE ADVANTAGE of controlling the delegate implementation. We have one that is optimized for JBoss and one that will work in any transaction manager. I do not want to introduce global synchronizations again. I spent a 3-4 weeks finding and removing global synchronizations a few years ago, and I do not want myself or somebody else to have to do it again. We should be limiting global synchronization whereever we can and TransactionLocal is definately a place we can do this.

              • 4. Re: mt-issues with org.jboss.tm.TransactionLocal

                 

                "bill.burke@jboss.com" wrote:

                Yes, we do control the delegate implementation.


                No we don't. We allow any pluggable transaction manager to implement
                TransactionLocalDelegate (with the TransactionLocalDelegateImpl if they don't).

                • 5. Re: mt-issues with org.jboss.tm.TransactionLocal

                  The real issue here, is what are the implications of somebody doing
                  "get" twice in competing threads, or get/set - including set(null).

                  If there really is an issue of synchronization, it should be left to the
                  user of the TransactionLocal, who knows best what the scope is.

                  e.g. they might have code like the following which obviously needs an external
                  (reentrant) lock to be threadsafe:

                  TransactionLocal x = new TransactionLocal();
                  
                  if (x.get() == someValue)
                   x.set(anotherValue);
                  


                  The caller will know best what the scope is.
                  i.e. I own this transaction for this critical section.


                  • 6. Re: mt-issues with org.jboss.tm.TransactionLocal
                    bill.burke

                     

                    "adrian@jboss.org" wrote:
                    "bill.burke@jboss.com" wrote:

                    Yes, we do control the delegate implementation.


                    No we don't. We allow any pluggable transaction manager to implement
                    TransactionLocalDelegate (with the TransactionLocalDelegateImpl if they don't).


                    I understand that, but we do control the implementation of 99.99999% of used TransactionLocalDelegate implementations which was my point....The design of TransactionLocal should take advantage of this and not be designed for the 0.00001% of cases.

                    • 7. Re: mt-issues with org.jboss.tm.TransactionLocal

                      I'm also arguing that the external lock will probably be transaction scoped.

                      • 8. Re: mt-issues with org.jboss.tm.TransactionLocal

                         

                        "adrian@jboss.org" wrote:
                        I'm also arguing that the external lock will probably be transaction scoped.


                        If the operation is already controlled by am equivalent or tighter lock,
                        anything inside TransactionLocal is redundant, even if it doesn't
                        suffer from the synchronize and callout anti-pattern I mentioned above.

                        • 9. Re: mt-issues with org.jboss.tm.TransactionLocal

                           

                          "bill.burke@jboss.com" wrote:

                          I understand that, but we do control the implementation of 99.99999% of used TransactionLocalDelegate implementations which was my point....The design of TransactionLocal should take advantage of this and not be designed for the 0.00001% of cases.


                          Yes, but your argument applies to the delegate callouts, where I used the qualification
                          "or even"
                          The initialValue() callout is "user code".

                          I don't see how plain synchronization on the TransactionLocal (which is a global object
                          across all transactions) will ever be the correct thing to do. If you need to do that,
                          you probably shouldn't be using a TransactionLocal :-)

                          • 10. Re: mt-issues with org.jboss.tm.TransactionLocal
                            bill.burke

                             

                            "adrian@jboss.org" wrote:
                            "bill.burke@jboss.com" wrote:

                            I understand that, but we do control the implementation of 99.99999% of used TransactionLocalDelegate implementations which was my point....The design of TransactionLocal should take advantage of this and not be designed for the 0.00001% of cases.


                            Yes, but your argument applies to the delegate callouts, where I used the qualification
                            "or even"
                            The initialValue() callout is "user code".

                            I don't see how plain synchronization on the TransactionLocal (which is a global object
                            across all transactions) will ever be the correct thing to do. If you need to do that,
                            you probably shouldn't be using a TransactionLocal :-)


                            Maybe you didn't read my above email. I'm suggesting that any synchronization done should be done in the TransactionLocalDelegate implementation. For our implementation, we can do it in org.jboss.tm.TransactionImpl.

                            • 11. Re: mt-issues with org.jboss.tm.TransactionLocal

                               

                              "bill.burke@jboss.com" wrote:
                              Maybe you didn't read my above email. I'm suggesting that any synchronization done should be done in the TransactionLocalDelegate implementation. For our implementation, we can do it in org.jboss.tm.TransactionImpl.


                              Yes, I read your previous response.

                              1) I don't disagree that the delegate needs to be internally threadsafe.
                              This is not true at the moment:
                              a) TransactionImpl has an unsynchronized HashMap to store the values.
                              b) TransactionLocalDelegate.getSynchronization() could register two synchronizations

                              2) I disagree that TransactionLocal needs to do internal synchronization, it should
                              just be marked as not threadsafe.
                              e.g. this code could lead to deadlocks

                              public class Blah
                              {
                               static TransactionLocal local = new TransactionLocal()
                               {
                               protected Object initialValue()
                               {
                               return doSomeExternalCalculationThatSynchronizesOnWhoKnowsWhat();
                               }
                               }
                              
                               public void doSomething()
                               {
                               Object value = local.get();
                               }
                              }
                              


                              The ".get()" could invoke the initialValue() with contradictory synchronizations
                              to the way doSomething() is used, and you also have the "unknown" synchronization
                              on the delegate that the user can't see.
                              This is just a mess.

                              3) Leave it to the user to correctly synchronize (they know best!)

                              • 12. Re: mt-issues with org.jboss.tm.TransactionLocal

                                4) Your also going to get into further trouble with these locks competing
                                with other locks for correct ordering, like TransactionImpl's lock()/unlock().

                                • 13. Re: mt-issues with org.jboss.tm.TransactionLocal
                                  dimitris

                                  So from your postings and the code, I gather:

                                  1) TransactionLocal is mt-safe when accessed by threads in different transactions, but non-mt-safe when accessed by threads in the same transaction.

                                  2) To avoid global contention any synchronization would need to happen in the TransactionLocalDelegate implentation, at tx level, with a trimming-dowm of TransactionLocal, as Bill suggested.

                                  3) However, Adrian points out that the caller is really the one who defines the mt access pattern, which most probably would be tx scoped, so it should be his responsibility for applying locking, if needed.

                                  My take is that if we don't have known/reported issues of mt problems with TransactionLocal, we are better-off documenting its mt behaviour described in (1), without introducing extra snychronization.

                                  • 14. Re: mt-issues with org.jboss.tm.TransactionLocal

                                    A further option would be to provide the locking described by Bill,
                                    but make it optional, i.e. in TransactionLocal

                                    public void lock();
                                    public void unlock();
                                    


                                    This can then be used by the caller and/or we can provide a subclass that does it
                                    automatically if the user doesn't want to think too hard. :-)
                                    public class SynchronizedTransactionLocal extends TransactionLocal
                                    {
                                    ...
                                     public Object get()
                                     {
                                     lock();
                                     try
                                     {
                                     return super.get();
                                     }
                                     finally
                                     {
                                     unlock();
                                     }
                                     }
                                    ...
                                    }
                                    


                                    1 2 Previous Next