DWR

Mike have you been able to review the changes I made? Any time to push out a 2.06 in the coming weeks?

I've looked at your changes now and I think it probably looks good.
There are a number of places where the (now synchronized) converters collection may change under our feet, such as in
getConverterAssignableFrom(Class paramType)
where there are a number of interspersed converters.get and converters.put without locking the collection in between calls.
Though, this may not be so bad as two threads conflicting here will be adding the same converter objects anyway, as we are normally only adding (not removing) obejcts to the map. You may want to ping Tim and double-check what he thinks, just in case.

converters is a synchronizedMap and there are no compound actions in getConverterAssignableFrom thus this should be safe. But I agree, I will have Tim take another look.

I consider getConverterAssignableFrom a compound operation on the converters map. In f ex the following scenario, the converters collection will be accessed five times during one call to the method:

class A {...}
class B extends A {...}
class C extends B {...}
<convert match="A"/>

getConverterAssignableFrom(C)
converters.get("C") // null
converters.get("B") // null
converters.get("A") // ConverterA
converters.put("B", ConverterA)
converters.put("C", ConverterA)

Two threads could both be executing getConverterAssignableFrom(C) and could be interspersed so that both get:
converters.get("B") == null
and then both set:
converters.put("B", ConverterA)
without ever knowing about the other thread. With any other kind of collection this would be a bug, and we would need to synchronize the whole getConverterAssignableFrom method. But as this is a map, and the same objects are inserted independent of how many times we do it, then this is probably ok. But still, any additional insights from Tim would be interesting to hear.

Tim, I hope you don't mind me adding you as a watcher. Comments?

Not sure what I was thinking here. I see the issue, I don't think we need to synchronize the entire method though. I will try again tonight.

I don't mind being added at all!

In DefaultConverterManager.java:

I don't think there's a problem, assuming addConverterType, addConverter and setConverters are only called during servlet init. I tried to construct a pathological case where you get different states of the converters map depending on the order that getConverterAssignableFrom is called on various Class objects, but I couldn't do it. Even if one could construct such a monster, it'd be hard to argue that any of the possible states was actually wrong, and easy to argue that the creator of such a mess would deserve the resulting (very mild) nondeterminism. Am I missing something obvious?

So DefaultConverterManager looks OK to me. I was going to say that you need to restore the synchronized block in setConverters:

public void setConverters(Map<String, Converter> converters)
{
synchronized (this.converters)

}

to defend against concurrent calls to setConverters, but if there's a guarantee that it will be called at most once, during init, then you don't need the synchronized block. (If you have it, though, better to lock this.converters rather than this.)

You should take out the GuardedBy("this") comment for DefaultScriptSession.attributes. It's a final reference to a thread-safe object, so it doesn't need to be (and isn't) guarded.

In DefaultScriptSession.java:

I don't know how DefaultScriptSession.getAttributeNames is used, but if the returned iterator needs to be thread-safe or stable, I would copy the attributes key set and return an unmodifiable iterator over the results. If the result is used in a single thread and it's OK if changes occur during iteration (or if you know that they won't), then it's fine as is – ConcurrentHashMap iterators won't throw ConcurrentModificationException.

You must synchronize on conduits rather than this in addScript and countPersistentConnections and change the conduits field comment to GuardedBy("self").

Likewise you must synchronize on scripts rather than this in writeScripts and change the scripts field comment to GuardedBy("self"). Better yet, if scripts is mostly read/iterated and rarely modified, use CopyOnWriteArrayList and get rid of synchronization entirely for the scripts field.

I don't think you want the synchronized block in removeScriptConduit.

You aren't making essential use of AtomicLong or AtomicBoolean with lastAccessedTime and invalidated. They might as well be volatile. The only reason to use the Atomic flavors is if you need an atomic operation like compareAndSet.

Similarly, you don't need to declare attributes as a ConcurrentMap, since you aren't using any ConcurrentMap operations. It's fine as a Map. (Same goes for converters in DefaultConverterManager, btw.)

Thanks Tim. I just checked in a change to both files that covers the majority of your recommendations. I would also like for you to take a quick look at DefaultScriptSessionManager - looks like we can certainly make some improvements.

I don't really understand what's going in DefaultScriptSessionManager, but it looks as though you could replace the three HashMap fields with ConcurrentHashMap and remove sessionLock entirely. The reason not to would be if those synchronized blocks have to execute atomically.

Here's an example: Concurrent calls to associateScriptSessionAndPage method could result in two near-simultaneous calls to put, with one of the scriptSession values getting lost. The fact that this hasn't been flagged as a bug suggests that associateScriptSessionAndPage does not get called concurrently, but if it really is a bug, you could initialize pageSessionMap as a CHM, declare it as ConcurrentMap, and use putIfAbsent instead of put in associateScriptSessionAndPage.

scriptSessionListeners looks unsafe, and I don't see why EventListenerList is being used. Why not make it a List<ScriptSessionListener> and initialize it with CopyOnWriteArrayList<ScriptSessionListener>?

lastSessionCheckAt should be volatile if maybeCheckTimeouts can be called from multiple threads.

future should be volatile unless afterContainerSetup is only called during init, which seems likely.

The rest of the settable fields are OK if their setters are only called during init.

Back in DefaultScriptSession, should the last two statements of invalidate() be in the finally clause of a try-finally, to defend against the possibility that listener.valueUnbound will throw an exception? Or should the call to valueUnbound be wrapped in try-finally so that the loop can continue?

Expanding on the use of putIfAbsent:

Set<DefaultScriptSession> pageSessions = pageSessionMap.get(normalizedPage);
if (pageSessions == null)
{
pageSessions = new HashSet<DefaultScriptSession>();
Set<DefaultScriptSession> prev = pageSessionMap.putIfAbsent(normalizedPage, pageSessions);
if (prev != null)

}

pageSessions.add(scriptSession);

Guys, I am going to resolve this issue as it was originally written for DefaultConverterManager. I have created a new issue (DWR-437) More Concurrency Issues to cover Tim's additional comments so we get them all in.