Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: EE-1.8.2.GA_P07
-
Fix Version/s: EE-1.8.2.GA_P08
-
Component/s: Framework
-
Labels:None
-
Environment:WebLogic Seam
-
Salesforce Case Reference:
Description
Activity
- All
- Comments
- History
- Activity
- Remote Attachments
- Subversion
It would appear that this is a regression from the fix for ICE-9242.
The changes in ICE-8833 may also have had an impact as they changed when the the sessionMonitor.shutdown() call was made. It now occurs immediately rather than wait for the view to be released at the end of the lifecycle.
A couple of approaches to fixing this. One is to institute a lock ordering to ensure we grab the appropriate lock first. This would entail grabbing the SessionMonitors lock before the context lock when shutting down:
public void shutdown() { //notify all the contexts associated to this monitored session synchronized (SessionMonitors){ synchronized (contexts){ Iterator i = contexts.iterator();
However this adds additional overhead. Mircea suggested what is likely a better way. Moving the call to addInSessionContext() outside of the block synchronized on SessionMonitors:
synchronized (SessionMonitors) { if (!SessionMonitors.containsKey(id)) { monitor = new Monitor(id, session); SessionMonitors.put(id, monitor); } else { monitor = (Monitor) SessionMonitors.get(id); } } //it is possible to have multiple web-app contexts associated with the same session ID monitor.addInSessionContext(context);
This removes the interleaving while reducing (rather than expanding) our synchronization footprint. So I'm going with the second one.
New thread dump for recent issue.
Looks like the original idea of lock ordering would have been the better choice and this is what the customer patch does. Our original fix did solve the immediate problem - a deadlock on checkSession() - but it just left another point where the locks interleaved. So I've gone over the thread dump and the code and added in the lock ordering pretty much as outlined in the client's suggested patch.
I think the performance hit should be minimal and synchronizing in the catch block, while unorthodox, seems to be okay considering that the catch block is being used to simply catch when an operation on the session is invalid because the session has expired, necessitating that it be safely removed anyway.
Committed to trunk as revision 38803.
Patch as applied to trunk
Customer patch applied. Resolving as fixed.
Verified session expiry on WebLogic8.1 (facelets), JBoss5.1 (jsp), WebLogic12.1.3, glassfish 3.1.2, tomcat 7 (facelets-enh) with component-showcase in IE11, FF34, Chrome43.
Verified session expiry with concurrent users WebLogic8.1, WebLogic 12.1.3 with component-showcase (facelets).
This is the invalidation thread involved in the deadlock:
And this is the other:
In thread '49', the session has been invalidated and the SessionDispatcher is attempting to shut down the Monitor for that session. In the process of doing that it grabs the lock on the contexts instance (a synchronized Set):
As part of notifySessionShutdown(), it wants to grab the lock for the SessionMonitors (a plain HashMap):
In thread '35', we see that it grabbed the lock for the SessionMonitors:
But as part of calling addInSessionContext(), it needs the lock on the contexts (a synchronized Set) instance being held by thread '49':
public void addInSessionContext(ServletContext context) { contexts.add(context); }Subsequent request threads just start piling up waiting on the lock for the SessionMonitors: