mt-issues with org.jboss.tm.TransactionLocal
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.
