Concurrency Issue

Description

DefaultConverterManager.convertInbound which is called during requests calls DefaultConverterManager.getConverterAssignableFrom which mutates the converters map in an unsyncrhonized manner.

See for more discussion: [dwr-user]ConcurrentModificationException on DefaultConverterManager.getConverterAssignableFrom

Activity

Show:
Tim Peierls
December 1, 2009, 11:51 AM

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)
{
this.converters.clear();
this.converters.putAll(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.)

David Marginian
December 1, 2009, 6:47 PM

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.

Tim Peierls
December 1, 2009, 7:23 PM

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?

Tim Peierls
December 1, 2009, 7:25 PM

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 = prev;
}
}

pageSessions.add(scriptSession);

David Marginian
December 10, 2009, 6:44 PM

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

Assignee

David Marginian

Reporter

David Marginian

Labels

None

Documentation Required

No

Components

Fix versions

Priority

Critical
Configure