Menu
▾
▴
jboss-dev-forums
|
From: <ad...@jb...> - 2005-05-31 22:28:09
|
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.
View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879656#3879656
Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879656
|
|
From: <bil...@jb...> - 2005-05-31 22:46:38
|
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) {
| ...
| }
| }
| }
|
View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879659#3879659
Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879659
|
|
From: <bil...@jb...> - 2005-05-31 22:50:59
|
"ad...@jb..." 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. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879661#3879661 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879661 |
|
From: <ad...@jb...> - 2005-05-31 22:57:28
|
"bil...@jb..." 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). View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879663#3879663 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879663 |
|
From: <ad...@jb...> - 2005-05-31 23:07:30
|
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. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879666#3879666 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879666 |
|
From: <bil...@jb...> - 2005-05-31 23:08:11
|
"ad...@jb..." wrote : "bil...@jb..." 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. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879668#3879668 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879668 |
|
From: <ad...@jb...> - 2005-05-31 23:08:37
|
I'm also arguing that the external lock will probably be transaction scoped. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879669#3879669 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879669 |
|
From: <ad...@jb...> - 2005-05-31 23:14:21
|
"ad...@jb..." 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. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879672#3879672 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879672 |
|
From: <ad...@jb...> - 2005-05-31 23:28:02
|
"bil...@jb..." 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 :-) View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879675#3879675 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879675 |
|
From: <bil...@jb...> - 2005-05-31 23:34:44
|
"ad...@jb..." wrote : "bil...@jb..." 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. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879677#3879677 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879677 |
|
From: <ad...@jb...> - 2005-06-01 00:00:44
|
"bil...@jb..." 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!)
View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879684#3879684
Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879684
|
|
From: <ad...@jb...> - 2005-06-01 00:03:09
|
4) Your also going to get into further trouble with these locks competing with other locks for correct ordering, like TransactionImpl's lock()/unlock(). View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879685#3879685 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879685 |
|
From: <dim...@jb...> - 2005-06-01 13:04:36
|
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. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879750#3879750 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879750 |
|
From: <ad...@jb...> - 2005-06-01 17:17:56
|
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();
| }
| }
| ...
| }
|
View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879783#3879783
Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879783
|
|
From: <bil...@jb...> - 2005-06-01 17:59:57
|
lock()/unlock() should not be used internally in the TransactionLocal implementation as locking is not needed. TransactionLocal does need to provide a lock()/unlock() function so that you can protect the transaction local for multi-threaded transaction access within your code. Like, if in your application code, you need to check and set the tx local. If get() == null then set. You'll want to local.lock() if (get() == null) then set local.unlock(). View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879791#3879791 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879791 |
|
From: <dim...@jb...> - 2005-06-01 22:54:19
|
Ok, lock()/unlock() similar to TransactionImpl was added, along with a helper SynchronizedTransactionLocal class (in the branch I'm working). Thanks View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879852#3879852 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879852 |
|
From: <dim...@jb...> - 2005-06-02 13:19:31
|
Actually the locking needs to be smarter than the one in TransactionImp because we want to "lock" the ThreadLocal based on the active transaction, i.e. synchronize the threads inside the same tx only.
My implementation is a bit naive for the time being, due to the notifyAll().
I guess I need to look for an EDU.oswego equivalent.
| ...
| /**
| * Set to monitor which transactions are "locked"
| * with lock() and unlock().
| */
| private static final Set txLockSet = new HashSet();
| ...
| /**
| * Lock using the current transaction
| */
| public void lock()
| {
| lock(getTransaction());
| }
|
| /**
| * Lock using the provided transaction
| *
| * @param transaction
| */
| public void lock(Transaction transaction)
| {
| if (transaction == null)
| {
| throw new IllegalStateException("there is no transaction");
| }
|
| synchronized (txLockSet)
| {
| while (!txLockSet.add(transaction))
| {
| try
| {
| txLockSet.wait();
| }
| catch (InterruptedException ex)
| {
| // ignore
| }
| }
| // got the lock
| }
| }
|
| /**
| * Unlock using the current transaction
| */
| public void unlock()
| {
| unlock(getTransaction());
| }
|
| public void unlock(Transaction transaction)
| {
| if (transaction == null)
| {
| throw new IllegalStateException("there is no transaction");
| }
|
| synchronized (txLockSet)
| {
| if (!txLockSet.remove(transaction))
| {
| throw new IllegalStateException("Unlocking, but not locked");
| }
| // REVISE
| txLockSet.notifyAll();
| }
| }
|
View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879924#3879924
Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879924
|
|
From: <dim...@jb...> - 2005-06-02 13:30:39
|
Another thing, txLockSet shouldn't be static, since we protect the state of each particular TreadLocal per tx. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879928#3879928 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879928 |
|
From: <ad...@jb...> - 2005-06-02 14:30:54
|
ReentrantLock.
You don't want to ignore/eat InterruptedException.
It is thrown for a reason.
I hate the early java examples where people didn't know what to do with InterruptedException and just ignored it.
If you want to continue through the InterruptedException you should restore
the state of the thread to interrupted.
| boolean interrupted = false;
| synchronized (txLockSet)
| {
| while (!txLockSet.add(transaction))
| {
| try
| {
| txLockSet.wait();
| }
| catch (InterruptedException ex)
| {
| interrupted = true;
| }
| }
| if (interrupted)
| Thread.interrupt();
| // got the lock
| }
| }
|
View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879949#3879949
Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879949
|
|
From: <ad...@jb...> - 2005-06-02 14:32:26
|
Also, the current TransactionLocal.get() allows a null transaction (returning null). So should interpret the lock()/unlock() as noops when there is no transaction. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879951#3879951 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879951 |
|
From: <ad...@jb...> - 2005-06-02 14:35:39
|
"dim...@jb..." wrote : Another thing, txLockSet shouldn't be static, since we protect the state of each particular TreadLocal per tx. Correct. NO GLOBAL LOCKS. It is transaction + object (threadlocal). You might also consider adding a timeout to the lock such that we don't wait forever. e.g. if you are waiting 5 minutes to get access to the thread local it is almost certainly a deadlock. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879952#3879952 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879952 |
|
From: <dim...@jb...> - 2005-06-02 14:41:46
|
Thanks a lot for the hints!
Actually we make the same mistake in TransactionImpl :)
| ...
| try
| {
| // Wakeup happens when:
| // - notify() is called from unlock()
| // - notifyAll is called from instanceDone()
| wait();
| }
| catch (InterruptedException ex)
| {
| // ignore
| }
| ...
|
View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879953#3879953
Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879953
|
|
From: <ad...@jb...> - 2005-06-02 14:50:42
|
"dim...@jb..." wrote : | My implementation is a bit naive for the time being, due to the notifyAll(). | notify leaves it to the underlying lock implementation to decide which thread gets woken (e.g. this might implement fair queuing) notifyAll wakes up all the threads and leaves it to a race in the OS scheduler as to who aquires the lock. Semantically they should be same in this circumstance, with notify being more efficient. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879955#3879955 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879955 |
|
From: <ad...@jb...> - 2005-06-02 14:52:05
|
"dim...@jb..." wrote : Thanks a lot for the hints! | | Actually we make the same mistake in TransactionImpl :) | Then fix it :-) Or at least report it on JIRA if you don't have time to analyse/test the consequences. View the original post : http://www.jboss.org/index.html?module=bb&op=viewtopic&p=3879956#3879956 Reply to the post : http://www.jboss.org/index.html?module=bb&op=posting&mode=reply&p=3879956 |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X