From b37813257a514752b84b7dc3f77a2ec517455fff Mon Sep 17 00:00:00 2001
From: zzz <zzz@mail.i2p>
Date: Thu, 28 Jul 2011 18:26:21 +0000
Subject: [PATCH]   * TunnelPoolManager: Reduce race window for restarting a
 tunnel pool

---
 history.txt                                       |  5 +++++
 router/java/src/net/i2p/router/RouterVersion.java |  2 +-
 .../net/i2p/router/tunnel/pool/TunnelPool.java    |  6 ++++--
 .../i2p/router/tunnel/pool/TunnelPoolManager.java | 15 ++++++++++++---
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/history.txt b/history.txt
index 11a3217db8..d46297ab91 100644
--- a/history.txt
+++ b/history.txt
@@ -1,3 +1,8 @@
+2011-07-28 zzz
+  * Context: Split up big lock to avoid deadlocks
+  * Streaming: Avoid a rare exception on race
+  * TunnelPoolManager: Reduce race window for restarting a tunnel pool
+
 2011-07-27 kytv
   * Add armel (armv5tejl) wrapper. Compiled and tested in Debian Squeeze.
 
diff --git a/router/java/src/net/i2p/router/RouterVersion.java b/router/java/src/net/i2p/router/RouterVersion.java
index ea17ad55f3..e1f87e077d 100644
--- a/router/java/src/net/i2p/router/RouterVersion.java
+++ b/router/java/src/net/i2p/router/RouterVersion.java
@@ -18,7 +18,7 @@ public class RouterVersion {
     /** deprecated */
     public final static String ID = "Monotone";
     public final static String VERSION = CoreVersion.VERSION;
-    public final static long BUILD = 15;
+    public final static long BUILD = 16;
 
     /** for example "-test" */
     public final static String EXTRA = "";
diff --git a/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java b/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java
index d9344f0de9..b68cd9de6b 100644
--- a/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java
+++ b/router/java/src/net/i2p/router/tunnel/pool/TunnelPool.java
@@ -33,7 +33,7 @@ public class TunnelPool {
     private final List<TunnelInfo> _tunnels;
     private final TunnelPeerSelector _peerSelector;
     private final TunnelPoolManager _manager;
-    private boolean _alive;
+    private volatile boolean _alive;
     private long _lifetimeProcessed;
     private TunnelInfo _lastSelected;
     private long _lastSelectionPeriod;
@@ -64,7 +64,8 @@ public class TunnelPool {
      *  Warning, this may be called more than once
      *  (without an intervening shutdown()) if the
      *  tunnel is stopped and then restarted by the client manager with the same
-     *  Destination (i.e. for servers or clients w/ persistent key)
+     *  Destination (i.e. for servers or clients w/ persistent key,
+     *  or restarting close-on-idle clients)
      */
     void startup() {
         synchronized (_inProgress) {
@@ -362,6 +363,7 @@ public class TunnelPool {
     
         if (getTunnelCount() <= 0 && !isAlive()) {
             // this calls both our shutdown() and the other one (inbound/outbound)
+            // This is racy - see TunnelPoolManager
             _manager.removeTunnels(_settings.getDestination());
         }
     }
diff --git a/router/java/src/net/i2p/router/tunnel/pool/TunnelPoolManager.java b/router/java/src/net/i2p/router/tunnel/pool/TunnelPoolManager.java
index 487e3493d2..69c1d8f3c5 100644
--- a/router/java/src/net/i2p/router/tunnel/pool/TunnelPoolManager.java
+++ b/router/java/src/net/i2p/router/tunnel/pool/TunnelPoolManager.java
@@ -286,6 +286,7 @@ public class TunnelPoolManager implements TunnelManagerFacade {
         // should we share the clientPeerSelector across both inbound and outbound?
         // or just one for all clients? why separate?
 
+        boolean delayOutbound = false;
         // synch with removeTunnels() below
         synchronized (this) {
             inbound = _clientInboundPools.get(dest);
@@ -301,12 +302,18 @@ public class TunnelPoolManager implements TunnelManagerFacade {
                 outbound = new TunnelPool(_context, this, settings.getOutboundSettings(), 
                                           new ClientPeerSelector());
                 _clientOutboundPools.put(dest, outbound);
+                delayOutbound = true;
             } else {
                 outbound.setSettings(settings.getOutboundSettings());
             }
         }
         inbound.startup();
-        SimpleScheduler.getInstance().addEvent(new DelayedStartup(outbound), 3*1000);
+        // don't delay the outbound if it already exists, as this opens up a large
+        // race window with removeTunnels() below
+        if (delayOutbound)
+            SimpleScheduler.getInstance().addEvent(new DelayedStartup(outbound), 1000);
+        else
+            outbound.startup();
     }
     
     
@@ -331,8 +338,10 @@ public class TunnelPoolManager implements TunnelManagerFacade {
         if (_log.shouldLog(Log.DEBUG))
             _log.debug("Removing tunnels for the client " + destination);
         if (_context.clientManager().isLocal(destination)) {
-            if (_log.shouldLog(Log.CRIT))
-                _log.log(Log.CRIT, "wtf, why are you removing the pool for " + destination.toBase64(), new Exception("i did it"));
+            // race with buildTunnels() on restart of a client
+            if (_log.shouldLog(Log.ERROR))
+                _log.error("Not removing pool still registered with client manager: " + destination.toBase64(), new Exception("i did it"));
+            return;
         }
         TunnelPool inbound = _clientInboundPools.remove(destination);
         TunnelPool outbound = _clientOutboundPools.remove(destination);