mt-issues with org.jboss.tm.TransactionLocal
dimitris May 31, 2005 6:11 PMAlthough 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.