You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@beehive.apache.org by ek...@apache.org on 2006/04/12 22:30:12 UTC
svn commit: r393595 - in
/beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow:
PageFlowManagedObject.java internal/DeferredSessionStorageHandler.java
Author: ekoneil
Date: Wed Apr 12 13:30:10 2006
New Revision: 393595
URL: http://svn.apache.org/viewcvs?rev=393595&view=rev
Log:
Fix a NetUI deadlock problem that can occur very occasionally when the accept changes and ensure failover steps of the DeferredSessionStorageHandler are interleaved by two threads. This problem is tracked in BEEHIVE-1099 and discussed in this thread:
http://mail-archives.apache.org/mod_mbox/beehive-dev/200603.mbox/%3ce9ac83540603290941p3c614c9dn2c65f57dec8dfafe@mail.gmail.com%3e
The problem basically occurs when Thread1 sets A.jpf in the session during accept changes, Thread2 overwrites it with B.jpf during accept changes, and Thread1 then re-sets A.jpf in the session causing the Page Flow "destroy" lifecycle method to run on B.jpf. This "destroy" call does not synchronize on B.jpf and can result in deadlock with another thread interacting with the same JPF.
This change makes this accept changes process atomic on the HttpSession thus serializing updates of NetUI attributes to the session.
BB: self
Test: NetUI pass
Modified:
beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/PageFlowManagedObject.java
beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/internal/DeferredSessionStorageHandler.java
Modified: beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/PageFlowManagedObject.java
URL: http://svn.apache.org/viewcvs/beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/PageFlowManagedObject.java?rev=393595&r1=393594&r2=393595&view=diff
==============================================================================
--- beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/PageFlowManagedObject.java (original)
+++ beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/PageFlowManagedObject.java Wed Apr 12 13:30:10 2006
@@ -127,13 +127,12 @@
*/
public void valueBound( HttpSessionBindingEvent event )
{
- _valueUnbound = false;
}
/**
* Callback when this object is removed from the user session. Causes {@link #onDestroy} to be called. This is a
* framework-invoked method that should not be called directly.
- */
+ */
public void valueUnbound( HttpSessionBindingEvent event )
{
//
@@ -142,19 +141,18 @@
//
ServletContext servletContext = getServletContext();
HttpSession session = event.getSession();
-
+
if ( servletContext == null && session != null )
{
servletContext = session.getServletContext();
}
- _valueUnbound = true;
-
if ( servletContext != null )
{
- if ( Handlers.get( servletContext ).getStorageHandler().allowBindingEvent( event ) )
+ if ( !_valueUnbound && Handlers.get( servletContext ).getStorageHandler().allowBindingEvent( event ) )
{
destroy( session );
+ _valueUnbound = true;
}
}
}
Modified: beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/internal/DeferredSessionStorageHandler.java
URL: http://svn.apache.org/viewcvs/beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/internal/DeferredSessionStorageHandler.java?rev=393595&r1=393594&r2=393595&view=diff
==============================================================================
--- beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/internal/DeferredSessionStorageHandler.java (original)
+++ beehive/trunk/netui/src/pageflow/org/apache/beehive/netui/pageflow/internal/DeferredSessionStorageHandler.java Wed Apr 12 13:30:10 2006
@@ -20,6 +20,7 @@
import org.apache.beehive.netui.pageflow.handler.StorageHandler;
import org.apache.beehive.netui.pageflow.RequestContext;
import org.apache.beehive.netui.pageflow.ServletContainerAdapter;
+import org.apache.beehive.netui.pageflow.PageFlowController;
import org.apache.beehive.netui.pageflow.scoping.ScopedServletUtils;
import org.apache.beehive.netui.util.logging.Logger;
@@ -49,7 +50,7 @@
implements StorageHandler
{
private static final Logger _log = Logger.getInstance(DeferredSessionStorageHandler.class);
-
+
private static final String CHANGELIST_ATTR = InternalConstants.ATTR_PREFIX + "changedAttrs";
private static final String FAILOVER_MAP_ATTR = InternalConstants.ATTR_PREFIX + "failoverAttrs";
@@ -86,7 +87,7 @@
if (_log.isTraceEnabled()) {
_log.trace("setAttribute: " + attrName + "=" + value);
}
-
+
HttpServletRequest request = ScopedServletUtils.getOuterRequest( ( HttpServletRequest ) context.getRequest() );
Object currentValue = request.getAttribute( attrName );
@@ -124,7 +125,7 @@
if (_log.isTraceEnabled()) {
_log.trace("removeAttribute: " + attrName);
}
-
+
HttpServletRequest request = ScopedServletUtils.getOuterRequest( ( HttpServletRequest ) context.getRequest() );
Object currentValue = request.getAttribute( attrName );
@@ -150,7 +151,7 @@
if (_log.isTraceEnabled()) {
_log.trace("getAttribute: " + attributeName);
}
-
+
HttpServletRequest request = ScopedServletUtils.getOuterRequest( ( HttpServletRequest ) context.getRequest() );
Object val = request.getAttribute( attributeName );
if ( val != null ) return val;
@@ -176,102 +177,151 @@
public void applyChanges( RequestContext context )
{
HttpServletRequest request = ScopedServletUtils.getOuterRequest( ( HttpServletRequest ) context.getRequest() );
+ assert request != null;
HttpSession session = request.getSession( false );
- if (_log.isDebugEnabled()) {
- _log.debug("Applying changes for request " + request.getRequestURI());
- }
-
//
// If the session doesn't exist, we don't want to recreate it. It has most likely been intentionally
// invalidated, since we did ensure its existance in getAttribute.
//
- if ( session == null ) return;
+ if ( session == null )
+ return;
- HashSet changedAttrs = getChangedAttributesList( request, false, true );
+ if (_log.isDebugEnabled())
+ _log.debug("Applying changes for request " + request.getRequestURI() +
+ ". Identity: " + System.identityHashCode(request));
- if ( changedAttrs != null )
- {
- //
- // Go through each changed attribute, and either write it to the session or remove it from the session,
- // depending on whether or not it exists in the request.
- //
- for ( Iterator i = changedAttrs.iterator(); i.hasNext(); )
+ //
+ // Synchronize on the HttpSession in order to make the operation of applying changes from a request into
+ // the session atomic. Atomicity is needed in order to ensure that the attributes set in the session via
+ // "session.setAttribute(...)" are the same ones present when the "ensureFailover" call is made against the
+ // ServletContainerAdapter.
+ //
+ synchronized(session) {
+
+ HashSet changedAttrs = getChangedAttributesList( request, false, true );
+ if ( changedAttrs != null )
{
- String attrName = ( String ) i.next();
- Object val = request.getAttribute( attrName );
+ //
+ // Ensure that the Page Flow in the request is run through its destroy lifecycle correctly
+ //
+ for(Iterator i = changedAttrs.iterator(); i.hasNext(); ) {
+ String attrName = (String)i.next();
+ Object val = request.getAttribute(attrName);
- if ( val != null )
- {
// Write it to the session, but only if the current value isn't already this value.
- Object currentValue = session.getAttribute( attrName );
+ Object currSessValue = session.getAttribute( attrName );
+
+ //
+ // Here, the value in the session is different than the value about to be persisted to the
+ // session. In some cases, this requires a PageFlowManagedObject lifecycle method to be
+ // invoked on that object.
+ //
+ if(currSessValue != null &&
+ val != currSessValue &&
+ val instanceof PageFlowController &&
+ val instanceof HttpSessionBindingListener) {
+ //
+ // In order to maintain the single-threaded semantics of PFMO destruction, it's necessary
+ // to synchronize on PFMO objects here.
+ //
+ HttpSessionBindingEvent event = new SessionBindingEvent(session, attrName, currSessValue);
+ synchronized(currSessValue) {
+ ((HttpSessionBindingListener)currSessValue).valueUnbound(event);
+ }
+ }
+ }
- if ( currentValue != val )
+ //
+ // Go through each changed attribute, and either write it to the session or remove it from the session,
+ // depending on whether or not it exists in the request.
+ //
+ for ( Iterator i = changedAttrs.iterator(); i.hasNext(); )
+ {
+ String attrName = ( String ) i.next();
+ Object val = request.getAttribute( attrName );
+
+ if ( val != null )
{
- if (_log.isTraceEnabled()) {
- _log.trace("Committing attribute " + attrName + " to the session.");
+ // Write it to the session, but only if the current value isn't already this value.
+ Object currentValue = session.getAttribute( attrName );
+
+ if ( currentValue != val )
+ {
+ if (_log.isTraceEnabled())
+ _log.trace("Committing attribute " + attrName + " to the session.");
+
+ // This ThreadLocal value allows others (e.g., an HttpSessionBindingListener like
+ // PageFlowManagedObject) that we're in the middle of committing changes to the session.
+ _isCommittingChanges.set( Boolean.TRUE );
+
+ try
+ {
+ session.setAttribute( attrName, val );
+ }
+ finally
+ {
+ _isCommittingChanges.set( Boolean.FALSE );
+ }
+
+ request.removeAttribute( attrName );
}
-
+ }
+ else
+ {
+ if (_log.isTraceEnabled())
+ _log.trace("Removing attribute " + attrName + " from the session.");
+
+ //
// This ThreadLocal value allows others (e.g., an HttpSessionBindingListener like
- // PageFlowManagedObject) that we're in the middle of committing changes to the session.
+ // PageFlowManagedObject) to query whether the Framework is in the middle of committing
+ // changes to the session
+ //
_isCommittingChanges.set( Boolean.TRUE );
try
{
- session.setAttribute( attrName, val );
+ session.removeAttribute( attrName );
}
finally
{
_isCommittingChanges.set( Boolean.FALSE );
}
-
- request.removeAttribute( attrName );
}
}
- else
+ }
+
+ //
+ // Now, run the ensureFailover step to make sure that clusterable containers do the right thing
+ // with attributes that need to be serialized for failover.
+ //
+ HashMap failoverAttrs = getFailoverAttributesMap( request, false, true );
+ if ( failoverAttrs != null )
+ {
+ ServletContainerAdapter sa = AdapterManager.getServletContainerAdapter( getServletContext() );
+
+ for ( Iterator i = failoverAttrs.entrySet().iterator(); i.hasNext(); )
{
- if (_log.isTraceEnabled()) {
- _log.trace("Removing attribute " + attrName + " from the session.");
- }
-
- //
- // This ThreadLocal value allows others (e.g., an HttpSessionBindingListener like
- // PageFlowManagedObject) to query whether the Framework is in the middle of committing
- // changes to the session
- //
- _isCommittingChanges.set( Boolean.TRUE );
+ Map.Entry entry = ( Map.Entry ) i.next();
+ if (_log.isTraceEnabled())
+ _log.trace("Ensure failover for attribute " + entry.getKey());
- try
- {
- session.removeAttribute( attrName );
+ // This ThreadLocal value allows others (e.g., an HttpSessionBindingListener like
+ // PageFlowManagedObject) that we're in the middle of committing changes to the session.
+ _isCommittingChanges.set(Boolean.TRUE);
+ try {
+ sa.ensureFailover( ( String ) entry.getKey(), entry.getValue(), request );
}
- finally
- {
- _isCommittingChanges.set( Boolean.FALSE );
+ finally {
+ _isCommittingChanges.set(Boolean.FALSE);
}
}
}
- }
+ } // release lock on the HttpSession
-
- //
- // Now go through the attributes we need to ensure-failover on.
- //
- HashMap failoverAttrs = getFailoverAttributesMap( request, false, true );
-
- if ( failoverAttrs != null )
- {
- ServletContainerAdapter sa = AdapterManager.getServletContainerAdapter( getServletContext() );
-
- for ( Iterator i = failoverAttrs.entrySet().iterator(); i.hasNext(); )
- {
- Map.Entry entry = ( Map.Entry ) i.next();
- if (_log.isTraceEnabled()) {
- _log.trace("Ensure failover for attribute " + entry.getKey());
- }
- sa.ensureFailover( ( String ) entry.getKey(), entry.getValue(), request );
- }
- }
+ if (_log.isDebugEnabled())
+ _log.debug("Completed applying changes for request " + request.getRequestURI() +
+ ". Identity: " + System.identityHashCode(request));
}
public void dropChanges(RequestContext context)
@@ -321,17 +371,29 @@
getFailoverAttributesMap( request, true, false ).put( attributeName, value );
}
+ public boolean allowBindingEvent( Object event )
+ {
+ return ! ( ( Boolean ) _isCommittingChanges.get() ).booleanValue();
+ }
+
private static HashSet getChangedAttributesList( ServletRequest request, boolean create, boolean remove )
{
HashSet set = ( HashSet ) request.getAttribute( CHANGELIST_ATTR );
+ //
+ // Create a new Set in which to store the changed attributes
+ //
if ( set == null && create )
{
set = new HashSet();
request.setAttribute( CHANGELIST_ATTR, set );
}
- if ( set!= null && remove ) request.removeAttribute( CHANGELIST_ATTR );
+ //
+ // Remove the list of changed attributes from the request
+ //
+ if ( set != null && remove )
+ request.removeAttribute( CHANGELIST_ATTR );
return set;
}
@@ -349,10 +411,5 @@
if ( map != null && remove ) request.removeAttribute( FAILOVER_MAP_ATTR );
return map;
- }
-
- public boolean allowBindingEvent( Object event )
- {
- return ! ( ( Boolean ) _isCommittingChanges.get() ).booleanValue();
}
}