[jboss-cvs] JBossRemoting/src/main/org/jboss/remoting ...
Ron Sigal
ron_sigal at yahoo.com
Mon Aug 6 21:43:40 EDT 2007
User: rsigal
Date: 07/08/06 21:43:40
Modified: src/main/org/jboss/remoting Tag: remoting_2_2_0_GA
LeasePinger.java
Log:
JBREM-783: Reorganized disconnectClient() and stopPing() to avoid nondeterminism.
Revision Changes Path
No revision
No revision
1.8.2.12.2.4 +117 -57 JBossRemoting/src/main/org/jboss/remoting/LeasePinger.java
(In the diff below, changes in quantity of whitespace are not shown.)
Index: LeasePinger.java
===================================================================
RCS file: /cvsroot/jboss/JBossRemoting/src/main/org/jboss/remoting/LeasePinger.java,v
retrieving revision 1.8.2.12.2.3
retrieving revision 1.8.2.12.2.4
diff -u -b -r1.8.2.12.2.3 -r1.8.2.12.2.4
--- LeasePinger.java 5 Aug 2007 19:57:28 -0000 1.8.2.12.2.3
+++ LeasePinger.java 7 Aug 2007 01:43:40 -0000 1.8.2.12.2.4
@@ -40,11 +40,15 @@
private String invokerSessionID = null;
private Map clients = new ConcurrentHashMap();
+ private Map closingClients = new ConcurrentHashMap();
private TimerTask timerTask = null;
private long pingPeriod = -1;
private int disconnectTimeout = DEFAULT_DISCONNECT_TIMEOUT;
private boolean started;
+ private boolean stopping;
+ private boolean stopped;
+ private Object disconnectLock = new Object();
// Constructors ---------------------------------------------------------------------------------
@@ -59,6 +63,7 @@
this.invokerSessionID = invokerSessionID;
this.pingPeriod = defaultLeasePeriod;
this.defaultPingPeriod = defaultLeasePeriod;
+ log.debug(this + " created LeasePinger: " + invokerSessionID);
}
// Public ---------------------------------------------------------------------------------------
@@ -86,38 +91,33 @@
public void stopPing()
{
- if(trace) { log.trace(this + " stopping lease timer"); }
+ // The division of removeClient() (synchronized by MicroRemoteClientInvoker)
+ // into removeClient() (synchronized by MicroRemoteClientInvoker) and
+ // disconnectClient() (not synchronized) has created some nondeterminism.
+ // In particular, it allows the following scenario:
+ //
+ // 1. Thread A calls removeClient() for the next to the last client.
+ // 2. Thread B calls removeClient() for the last client.
+ // 3. Thread B calls disconnectClient() for the last client.
+ // 4. Thread B calls stopPing(), destroying the Lease on the server side.
+ // 5. Thread A calls disconnectClient() for the next to the last client.
+ //
+ // However, Thread A's call to disconnectClient() would fail because the
+ // server has already destroyed the lease.
+ //
+ // The solution is for stopPing() to simply set a flag, stopping, to true.
+ // The first thread to enter disconnectClient() and determine that stopping
+ // is true and that it has disconnected the last client will then call
+ // disconnect(), which tells the server to destroy the Lease. (Note that
+ // this determination requires some synchronization to avoid the possibility
+ // that two or more threads try to contact the server.)
+
+ stopping = true;
if (timerTask != null)
{
timerTask.cancel();
timerTask = null;
-
- try
- {
- // sending request map with no ClientHolders will indicate to server
- // that is full disconnect (for client invoker)
- HashMap metadata = null;
-
- // If disconnectTimeout == 0, skip network i/o.
- if (disconnectTimeout != 0)
- {
- if (disconnectTimeout > 0)
- {
- metadata = new HashMap(1);
- metadata.put(ServerInvoker.TIMEOUT, Integer.toString(disconnectTimeout));
- }
- InvocationRequest ir =
- new InvocationRequest(invokerSessionID, null, "$DISCONNECT$", metadata, null, null);
- invoker.invoke(ir);
- }
- }
- catch (Throwable throwable)
- {
- RuntimeException e = new RuntimeException("Error tearing down lease with server.");
- e.initCause(throwable);
- throw e;
- }
}
}
@@ -161,7 +161,7 @@
boolean isLastClientLease = false;
ClientHolder holder = (ClientHolder)clients.remove(sessionID);
- if(trace) { log.trace(this + " removing client with session ID " + sessionID); }
+ if(trace) { log.trace(this + " removing client with session ID " + sessionID + " (" + clients.size() + ")"); }
if (holder == null)
{
@@ -207,15 +207,17 @@
}
}
- clients.put(sessionID, holder);
+ closingClients.put(sessionID, holder);
return isLastClientLease;
}
public void disconnectClient(String sessionID)
{
+ try
+ {
if(trace) { log.trace(this + " disconnecting client with session ID " + sessionID); }
- ClientHolder holder = (ClientHolder)clients.remove(sessionID);
+ ClientHolder holder = (ClientHolder)closingClients.get(sessionID);
if (holder != null)
{
@@ -234,16 +236,18 @@
try
{
invoker.invoke(ir);
+ if(trace) { log.trace(this + " sent out disconnect message to server for lease tied to client with session ID " + sessionID); }
}
catch (Throwable throwable)
{
log.error(this + " failed sending disconnect for client lease for " +
"client with session ID " + sessionID, throwable);
- return;
}
-
- if(trace) { log.trace(this + " sent out disconnect message to server for lease tied to client with session ID " + sessionID); }
}
+
+ // closingClients will be empty only after an attempt has been made to
+ // disconnect all clients.
+ closingClients.remove(sessionID);
}
else
{
@@ -251,6 +255,30 @@
+ sessionID + ", but no such lease was found");
}
}
+ finally
+ {
+ // The first thread to enter this synchronization block and find that
+ // stopping == true and closingClients is empty will tell the server
+ // to destroy the Lease. Any other threads that enter the block
+ // under the same conditions will find stopped == true and will
+ // return without attempting to contact the server.
+ synchronized (disconnectLock)
+ {
+ if (stopped)
+ return;
+
+ if (stopping && closingClients.isEmpty())
+ {
+ stopped = true;
+ }
+ }
+
+ if (stopped)
+ {
+ disconnect();
+ }
+ }
+ }
public Map getClients()
{
@@ -348,6 +376,38 @@
}
}
+ private void disconnect()
+ {
+ if(trace) { log.trace(this + " destroying lease"); }
+
+ try
+ {
+ // sending request map with no ClientHolders will indicate to server
+ // that is full disconnect (for client invoker)
+ HashMap metadata = null;
+
+ // If disconnectTimeout == 0, skip network i/o.
+ if (disconnectTimeout != 0)
+ {
+ if (disconnectTimeout > 0)
+ {
+ metadata = new HashMap(1);
+ metadata.put(ServerInvoker.TIMEOUT, Integer.toString(disconnectTimeout));
+ }
+
+ InvocationRequest ir =
+ new InvocationRequest(invokerSessionID, null, "$DISCONNECT$", metadata, null, null);
+ invoker.invoke(ir);
+ }
+ }
+ catch (Throwable throwable)
+ {
+ RuntimeException e = new RuntimeException("Error tearing down lease with server.");
+ e.initCause(throwable);
+ throw e;
+ }
+ }
+
// Inner classes --------------------------------------------------------------------------------
static private class LeaseTimerTask extends TimerTask
More information about the jboss-cvs-commits
mailing list