On 7/30/2010 1:50 PM, Gregg Wonderly wrote:
Patricia Shanahan wrote:
Gregg Wonderly wrote:
In the case of task manager, it helps asynchronous activity happening
on separate threads, understand how to manage data races such that
outcomes can be predictable no matter how scheduling occurs.
Here is where I disagree. TaskManager itself is not sequence-number
aware. Suppose T1 and T2 are two tasks with sequence numbers 1 and 2
respectively. T1 and T2 are of the same type, and their runAfter method
would consider them a match in everything it tests except for sequence
number.
T2 will return true for a runAfter test on a list containing only T1. T1
will return false for a runAfter test on a list containing only T2,
because of the sequence number test. Both return false on an empty list.
If they are always added to a TaskManager in the order T1 then T2, the
sequence number check is unnecessary. If a task appears in the runAfter
check collection and conflicts with the current task, it has a lower
sequence number than the current task.
The interesting case is T2 added before T1. Suppose the TaskManager is
empty and has a couple of waiting threads at the time T2 is added. T2
sees an empty runAfter check list, so it is immediately runnable and the
thread is notified. The TaskManager thread may have committed to running
T2 before T1 is added.
Is there a case that you can point at, where T2 is added before T1? I am
not aware of such a case with my casual knowledge of this part of the code.
I have found a case where T2 then T1 is possible, in
ServiceDiscoveryManager.
ServiceDiscoveryManager.LookupCacheImpl.taskSeqN is used to initialize
sequence number fields in some CacheTask subclasses that are then used
in runAfter methods.
ServiceDiscoveryManager.LookupCacheImpl.addProxyReg creates a task with
incremented taskSeqN inside a serviceIdMap synchronized block, but adds
it to the cacheTaskMgr outside the block.
public void addProxyReg(ProxyReg reg) {
RegisterListenerTask treg;
synchronized(serviceIdMap) {
treg = new RegisterListenerTask(reg, taskSeqN++);
}//end sync(serviceIdMap)
cacheTaskMgr.add(treg);
}//end LookupCacheImpl.addProxyReg
In the remaining sequence numbered CacheTask subclass cases, the
cacheTaskMgr.add call is done inside the same synchronized(serviceIdMap)
block as the taskSeqN increment.
There is a window in addProxyReg during which a LookupTask or
NotifyEventTask with a higher sequence number could be added before the
RegisterListenerTask task addProxyReg is adding. The LookupTask and
NotifyEventTask runAfter methods both wait for any RegisterEventTask
with a lower sequence number.
I believe this is a bug, but it will be hard to construct a test case
because it involves such a narrow timing window. I do recommend moving
the cacheTaskMgr.add(treg) statement inside the synchronized block.
Patricia
