From 2f2aa7f5a8f39a421fc5cd57c2281fe0e27e4fc7 Mon Sep 17 00:00:00 2001 From: zzz Date: Thu, 13 Nov 2014 20:12:55 +0000 Subject: [PATCH] I2PTunnel: - Fix bug that left server acceptor thread running after close - Add destroy() methods to release all resources when closing a tunnel for good, particularly the streaming timer threads - Use COWAL to prevent concurrency problems - Javadocs Streaming: - Don't return null from accept() any more; actually throw ConnectException as the javadocs have always specified - Throw ConnectException from accept() if interrupted; previously caught and ignored - Throw exceptions from ConnectionHandler.accept(), not higher up - Close ServerSocket when ConnectionManager is shut down - Synchronize setActive(), clear queue when starting to accept, better handling of calls that don't change state - Javadocs ConfigClientsHelper: Call isPluginRunning() less often PluginStarter: Simplify detection of active threads Above changes mostly in support of zzzot plugin implementing ClientApp and being able to shut down completely so there are no threads in its thread group, so /configclients will all show status as stopped. Previously, the I2PTunnelServer acceptor thread and one or more streaming timer threads would remain. --- .../java/src/net/i2p/i2ptunnel/I2PTunnel.java | 43 +++++++++------ .../i2p/i2ptunnel/I2PTunnelClientBase.java | 16 ++++++ .../net/i2p/i2ptunnel/I2PTunnelServer.java | 53 ++++++++++++++----- .../src/net/i2p/i2ptunnel/I2PTunnelTask.java | 41 +++++++++++++- .../net/i2p/i2ptunnel/TunnelController.java | 20 +++++++ .../i2p/i2ptunnel/TunnelControllerGroup.java | 19 ++++++- .../i2p/client/streaming/I2PServerSocket.java | 11 ++-- .../client/streaming/I2PSocketManager.java | 7 +++ .../i2p/router/web/ConfigClientsHelper.java | 6 +-- .../src/net/i2p/router/web/PluginStarter.java | 18 +++++-- .../streaming/impl/ConnectionHandler.java | 42 +++++++++++---- .../streaming/impl/ConnectionManager.java | 13 +++++ .../streaming/impl/I2PServerSocketFull.java | 18 +++++-- .../streaming/impl/I2PSocketManagerFull.java | 34 ++++++------ history.txt | 16 ++++++ .../src/net/i2p/router/RouterVersion.java | 2 +- 16 files changed, 283 insertions(+), 76 deletions(-) diff --git a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnel.java b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnel.java index 1bfbada56a..22379244af 100644 --- a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnel.java +++ b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnel.java @@ -52,6 +52,7 @@ import java.util.Map; import java.util.Properties; import java.util.Set; import java.util.StringTokenizer; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.CopyOnWriteArraySet; import java.util.concurrent.atomic.AtomicLong; @@ -107,7 +108,7 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { private static final String nocli_args[] = { "-nocli", "-die"}; - private final List tasks = new ArrayList(); + private final List tasks = new CopyOnWriteArrayList(); private int next_task_id = 1; private final Set listeners = new CopyOnWriteArraySet(); @@ -123,6 +124,9 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { new LongOpt("wait", LongOpt.NO_ARGUMENT, null, 'w') }; + /** @since 0.9.17 */ + private enum CloseMode { NORMAL, FORCED, DESTROY } + public static void main(String[] args) { try { new I2PTunnel(args); @@ -455,7 +459,7 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { " auth \n" + " client [, []\n" + " clientoptions [-acx] [key=value ]*\n" + - " close [forced] |all\n" + + " close [forced|destroy] |all\n" + " config [-s] \n" + " connectclient [] []\n" + " genkeys []\n" + @@ -1531,21 +1535,24 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { * * Sets the event "closeResult" = "ok" after the closing is complete * - * @param args {jobNumber}, {"forced", jobNumber}, {"forced", "all"} + * @param args {jobNumber}, {"forced", jobNumber}, {"forced", "all"}, {"destroy", jobNumber}, {"destroy", "all"} * @param l logger to receive events and output */ public void runClose(String args[], Logging l) { if (args.length == 0 || args.length > 2) { - l.log("close [forced] |all\n" + + l.log("close [forced|destroy] |all\n" + " stop running tasks. either only one or all.\n" + " use 'forced' to also stop tasks with active connections.\n" + " use the 'list' command to show the job numbers"); notifyEvent("closeResult", "error"); } else { int argindex = 0; // parse optional 'forced' keyword - boolean forced = false; + CloseMode mode = CloseMode.NORMAL; if (args[argindex].equalsIgnoreCase("forced")) { - forced = true; + mode = CloseMode.FORCED; + argindex++; + } else if (args[argindex].equalsIgnoreCase("destroy")) { + mode = CloseMode.DESTROY; argindex++; } if (args[argindex].equalsIgnoreCase("all")) { @@ -1555,7 +1562,7 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { _log.info(getPrefix() + " runClose(all) no tasks"); } for (I2PTunnelTask t : tasks) { - if (!closetask(t, forced, l)) { + if (!closetask(t, mode, l)) { notifyEvent("closeResult", "error"); error = true; } else if (!error) { // If there's an error, don't hide it @@ -1564,7 +1571,7 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { } } else { try { - if (!closetask(Integer.parseInt(args[argindex]), forced, l)) { + if (!closetask(Integer.parseInt(args[argindex]), mode, l)) { notifyEvent("closeResult", "error"); } else { notifyEvent("closeResult", "ok"); @@ -1669,7 +1676,7 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { * closure) * */ - private boolean closetask(int num, boolean forced, Logging l) { + private boolean closetask(int num, CloseMode mode, Logging l) { boolean closed = false; _log.debug(getPrefix() + "closetask(): looking for task " + num); @@ -1678,7 +1685,7 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { if (_log.shouldLog(Log.DEBUG)) _log.debug(getPrefix() + "closetask(): parsing task " + id + " (" + t.toString() + ")"); if (id == num) { - closed = closetask(t, forced, l); + closed = closetask(t, mode, l); break; } else if (id > num) { break; @@ -1692,17 +1699,23 @@ public class I2PTunnel extends EventDispatcherImpl implements Logging { * (optionally forcing closure) * */ - private boolean closetask(I2PTunnelTask t, boolean forced, Logging l) { + private boolean closetask(I2PTunnelTask t, CloseMode mode, Logging l) { if (_log.shouldLog(Log.INFO)) - _log.info("Closing task " + t.getId() + (forced ? " forced..." : "...")); + _log.info("Closing task " + t.getId() + " mode: " + mode); //l.log("Closing task " + t.getId() + (forced ? " forced..." : "...")); - if (t.close(forced)) { + boolean success; + if (mode == CloseMode.NORMAL) + success = t.close(false); + else if (mode == CloseMode.FORCED) + success = t.close(true); + else // DESTROY + success = t.destroy(); + if (success) { if (_log.shouldLog(Log.INFO)) _log.info("Task " + t.getId() + " closed."); //l.log("Task " + t.getId() + " closed."); - return true; } - return false; + return success; } /** diff --git a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelClientBase.java b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelClientBase.java index 3473a1831e..673928552f 100644 --- a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelClientBase.java +++ b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelClientBase.java @@ -796,6 +796,22 @@ public abstract class I2PTunnelClientBase extends I2PTunnelTask implements Runna return true; } + /** + * Note that the tunnel cannot be reopened after this by calling startRunning(), + * as it will destroy the underlying socket manager. + * This releases all resources if not a shared client. + * For shared client, the router will kill all the remaining streaming timers at shutdown. + * + * @since 0.9.17 + */ + @Override + public synchronized boolean destroy() { + close(true); + if (_ownDest) + sockMgr.destroySocketManager(); + return true; + } + public static void closeSocket(Socket s) { try { s.close(); diff --git a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelServer.java b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelServer.java index dfec31bc37..71d2643b22 100644 --- a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelServer.java +++ b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelServer.java @@ -29,6 +29,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.ThreadFactory; import net.i2p.I2PException; +import net.i2p.client.I2PSession; import net.i2p.client.I2PSessionException; import net.i2p.client.streaming.I2PServerSocket; import net.i2p.client.streaming.I2PSocket; @@ -45,7 +46,7 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { protected final Log _log; protected final I2PSocketManager sockMgr; - protected I2PServerSocket i2pss; + protected volatile I2PServerSocket i2pss; private final Object lock = new Object(); protected final Object slock = new Object(); @@ -213,7 +214,7 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { try { I2PSocketManager rv = I2PSocketManagerFactory.createDisconnectedManager(privData, getTunnel().host, portNum, props); - rv.setName("Server"); + rv.setName("I2PTunnel Server"); getTunnel().addSession(rv.getSession()); return rv; } catch (I2PSessionException ise) { @@ -317,6 +318,13 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { return readTimeout; } + /** + * Note that the tunnel can be reopened after this by calling startRunning(). + * This does not release all resources. In particular, the I2PSocketManager remains + * and it may have timer threads that continue running. + * + * To release all resources permanently, call destroy(). + */ public synchronized boolean close(boolean forced) { if (!open) return true; if (task != null) { @@ -331,16 +339,20 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { return false; } l.log("Stopping tunnels for server at " + this.remoteHost + ':' + this.remotePort); + open = false; try { - if (i2pss != null) i2pss.close(); - getTunnel().removeSession(sockMgr.getSession()); - sockMgr.getSession().destroySession(); + if (i2pss != null) { + i2pss.close(); + i2pss = null; + } + I2PSession session = sockMgr.getSession(); + getTunnel().removeSession(session); + session.destroySession(); } catch (I2PException ex) { _log.error("Error destroying the session", ex); //System.exit(1); } //l.log("Server shut down."); - open = false; if (_usePool && _executor != null) { _executor.setRejectedExecutionHandler(new ThreadPoolExecutor.DiscardPolicy()); _executor.shutdownNow(); @@ -349,6 +361,20 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { } } + /** + * Note that the tunnel cannot be reopened after this by calling startRunning(), + * as it will destroy the underlying socket manager. + * This releases all resources. + * + * @since 0.9.17 + */ + @Override + public synchronized boolean destroy() { + close(true); + sockMgr.destroySocketManager(); + return true; + } + /** * Update the I2PSocketManager. * And since 0.9.15, the target host and port. @@ -434,7 +460,7 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { * hands each I2P socket to the executor or runs it in-line. */ public void run() { - I2PServerSocket i2pS_S = sockMgr.getServerSocket(); + i2pss = sockMgr.getServerSocket(); if (_log.shouldLog(Log.WARN)) { if (_usePool) _log.warn("Starting executor with " + getHandlerCount() + " threads max"); @@ -446,7 +472,10 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { } while (open) { try { - final I2PSocket i2ps = i2pS_S.accept(); + I2PServerSocket ci2pss = i2pss; + if (ci2pss == null) + throw new I2PException("I2PServerSocket closed"); + final I2PSocket i2ps = ci2pss.accept(); if (i2ps == null) throw new I2PException("I2PServerSocket closed"); if (_usePool) { try { @@ -473,10 +502,8 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { } catch (ConnectException ce) { if (_log.shouldLog(Log.ERROR)) _log.error("Error accepting", ce); - // not killing the server.. - try { - Thread.sleep(500); - } catch (InterruptedException ie) {} + open = false; + break; } catch(SocketTimeoutException ste) { // ignored, we never set the timeout } catch (Exception e) { @@ -489,7 +516,7 @@ public class I2PTunnelServer extends I2PTunnelTask implements Runnable { } catch (InterruptedException ie) {} } } - if (_executor != null) + if (_executor != null && !_executor.isTerminating() && !_executor.isShutdown()) _executor.shutdownNow(); } diff --git a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelTask.java b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelTask.java index 2954b0a2f9..b59567ff89 100644 --- a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelTask.java +++ b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelTask.java @@ -8,9 +8,15 @@ import net.i2p.util.EventDispatcher; import net.i2p.util.EventDispatcherImpl; /** - * Either a Server or a Client. + * Either a Server or a Client. + * + * Use caution if extending externally. + * This class should be maintained as a stable API, + * but ask to be sure. + * + * Note that there is no startRunning() method, + * however all extending classes implement one. */ - public abstract class I2PTunnelTask extends EventDispatcherImpl { private int id; @@ -56,12 +62,37 @@ public abstract class I2PTunnelTask extends EventDispatcherImpl { tunnel.routerDisconnected(); } + /** + * Note that the tunnel can be reopened after this by calling startRunning(). + * This may not release all resources. In particular, the I2PSocketManager remains + * and it may have timer threads that continue running. + * + * To release all resources permanently, call destroy(). + * + * @return success + */ public abstract boolean close(boolean forced); + /** + * Note that the tunnel cannot be reopened after this by calling startRunning(), + * as it may destroy the underlying socket manager, depending on implementation. + * This should release all resources. + * + * The implementation here simply calls close(true). + * Extending classes should override to release all resources. + * + * @return success + * @since 0.9.17 + */ + public boolean destroy() { + return close(true); + } + /** * Notify the task that I2PTunnel's options have been updated. * Extending classes should override and call I2PTunnel.getClientOptions(), * then update the I2PSocketManager. + * Does nothing here. * * @since 0.9.1 */ @@ -80,9 +111,15 @@ public abstract class I2PTunnelTask extends EventDispatcherImpl { getTunnel().removeSession(session); } + /** + * Does nothing here. Extending classes may override. + */ public void errorOccurred(I2PSession session, String message, Throwable error) { } + /** + * Does nothing here. Extending classes may override. + */ public void reportAbuse(I2PSession session, int severity) { } diff --git a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelController.java b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelController.java index 452860a157..cb68319494 100644 --- a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelController.java +++ b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelController.java @@ -548,6 +548,11 @@ public class TunnelController implements Logging { } } + /** + * May be restarted with restartTunnel() or startTunnel() later. + * This may not release all resources. In particular, the I2PSocketManager remains + * and it may have timer threads that continue running. + */ public void stopTunnel() { // I2PTunnel removes the session in close(), // so save the sessions to pass to release() and TCG @@ -556,6 +561,21 @@ public class TunnelController implements Logging { release(sessions); _running = false; } + + /** + * May NOT be restarted with restartTunnel() or startTunnel() later. + * This should release all resources. + * + * @since 0.9.17 + */ + public void destroyTunnel() { + // I2PTunnel removes the session in close(), + // so save the sessions to pass to release() and TCG + Collection sessions = getAllSessions(); + _tunnel.runClose(new String[] { "destroy", "all" }, this); + release(sessions); + _running = false; + } public void restartTunnel() { stopTunnel(); diff --git a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelControllerGroup.java b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelControllerGroup.java index a0614dceba..af8818abdc 100644 --- a/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelControllerGroup.java +++ b/apps/i2ptunnel/java/src/net/i2p/i2ptunnel/TunnelControllerGroup.java @@ -268,7 +268,7 @@ public class TunnelControllerGroup implements ClientApp { * */ public synchronized void unloadControllers() { - stopAllControllers(); + destroyAllControllers(); _controllers.clear(); if (_log.shouldLog(Log.INFO)) _log.info("All controllers stopped and unloaded"); @@ -296,7 +296,7 @@ public class TunnelControllerGroup implements ClientApp { } /** - * Stop all tunnels + * Stop all tunnels. May be restarted. * * @return list of messages the tunnels generate when stopped */ @@ -312,6 +312,21 @@ public class TunnelControllerGroup implements ClientApp { return msgs; } + /** + * Stop all tunnels. They may not be restarted, you must reload. + * Caller must synch. Caller must clear controller list. + * + * @since 0.9.17 + */ + private void destroyAllControllers() { + for (int i = 0; i < _controllers.size(); i++) { + TunnelController controller = _controllers.get(i); + controller.destroyTunnel(); + } + if (_log.shouldLog(Log.INFO)) + _log.info(_controllers.size() + " controllers stopped"); + } + /** * Start all tunnels * diff --git a/apps/ministreaming/java/src/net/i2p/client/streaming/I2PServerSocket.java b/apps/ministreaming/java/src/net/i2p/client/streaming/I2PServerSocket.java index 35fb68daaa..3e9f7f1022 100644 --- a/apps/ministreaming/java/src/net/i2p/client/streaming/I2PServerSocket.java +++ b/apps/ministreaming/java/src/net/i2p/client/streaming/I2PServerSocket.java @@ -20,14 +20,15 @@ public interface I2PServerSocket { * Waits for the next socket connecting. If a remote user tried to make a * connection and the local application wasn't .accept()ing new connections, * they should get refused (if .accept() doesnt occur in some small period). - * Warning - unlike regular ServerSocket, may return null. + * Warning - unlike regular ServerSocket, may return null (through 0.9.16 only). * - * @return a connected I2PSocket OR NULL + * @return a connected I2PSocket OR NULL through 0.9.16; never null as of 0.9.17 * * @throws I2PException if there is a problem with reading a new socket - * from the data available (aka the I2PSession closed, etc) - * @throws ConnectException if the I2PServerSocket is closed - * @throws SocketTimeoutException + * from the data available (e.g. the I2PSession is closed) + * @throws ConnectException if the I2PServerSocket is closed, or if interrupted. + * Not actually thrown through 0.9.16; thrown as of 0.9.17 + * @throws SocketTimeoutException if a timeout was previously set with setSoTimeout and the timeout has been reached. */ public I2PSocket accept() throws I2PException, ConnectException, SocketTimeoutException; diff --git a/apps/ministreaming/java/src/net/i2p/client/streaming/I2PSocketManager.java b/apps/ministreaming/java/src/net/i2p/client/streaming/I2PSocketManager.java index 6ba93bf3b7..60367d9227 100644 --- a/apps/ministreaming/java/src/net/i2p/client/streaming/I2PSocketManager.java +++ b/apps/ministreaming/java/src/net/i2p/client/streaming/I2PSocketManager.java @@ -57,6 +57,13 @@ public interface I2PSocketManager { */ public I2PSocketOptions getDefaultOptions(); + /** + * Returns non-null socket. + * This method does not throw exceptions, but methods on the returned socket + * may throw exceptions if the socket or socket manager is closed. + * + * @return non-null + */ public I2PServerSocket getServerSocket(); /** diff --git a/apps/routerconsole/java/src/net/i2p/router/web/ConfigClientsHelper.java b/apps/routerconsole/java/src/net/i2p/router/web/ConfigClientsHelper.java index ab83bcbd20..5e5774cdca 100644 --- a/apps/routerconsole/java/src/net/i2p/router/web/ConfigClientsHelper.java +++ b/apps/routerconsole/java/src/net/i2p/router/web/ConfigClientsHelper.java @@ -261,9 +261,9 @@ public class ConfigClientsHelper extends HelperBase { .append("").append(_("Update link")).append(" "); } desc.append(""); - boolean enableStop = !Boolean.parseBoolean(appProps.getProperty("disableStop")); - enableStop &= PluginStarter.isPluginRunning(app, _context); - boolean enableStart = !PluginStarter.isPluginRunning(app, _context); + boolean isRunning = PluginStarter.isPluginRunning(app, _context); + boolean enableStop = isRunning && !Boolean.parseBoolean(appProps.getProperty("disableStop")); + boolean enableStart = !isRunning; renderForm(buf, app, app, false, "true".equals(val), false, false, desc.toString(), false, false, updateURL != null, enableStop, true, enableStart); diff --git a/apps/routerconsole/java/src/net/i2p/router/web/PluginStarter.java b/apps/routerconsole/java/src/net/i2p/router/web/PluginStarter.java index 0683ba6e62..87780bcee4 100644 --- a/apps/routerconsole/java/src/net/i2p/router/web/PluginStarter.java +++ b/apps/routerconsole/java/src/net/i2p/router/web/PluginStarter.java @@ -862,10 +862,22 @@ public class PluginStarter implements Runnable { ThreadGroup group = pluginThreadGroups.get(pluginName); if (group == null) return false; + boolean rv = group.activeCount() > 0; - Thread[] activeThreads = new Thread[1]; - group.enumerate(activeThreads); - return activeThreads[0] != null; + /**** debugging to figure out active threads + if (rv) { + Thread[] activeThreads = new Thread[32]; + int count = group.enumerate(activeThreads); + for (int i = 0; i < count; i++) { + if (activeThreads[i] != null) { + System.err.println("Found " + activeThreads[i].getState() + " thread for " + + pluginName + ": " + activeThreads[i].getName()); + } + } + } + ****/ + + return rv; } /** diff --git a/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionHandler.java b/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionHandler.java index 8c872a30d5..b14447f91d 100644 --- a/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionHandler.java +++ b/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionHandler.java @@ -1,5 +1,7 @@ package net.i2p.client.streaming.impl; +import java.net.ConnectException; +import java.net.SocketTimeoutException; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; @@ -43,16 +45,24 @@ class ConnectionHandler { _acceptTimeout = DEFAULT_ACCEPT_TIMEOUT; } - public void setActive(boolean active) { + public synchronized void setActive(boolean active) { + // FIXME active=false this only kills for one thread in accept() + // if they are more, they won't ket a poison packet. if (_log.shouldLog(Log.DEBUG)) _log.debug("setActive(" + active + ") called"); + // if starting, clear any old poison + // if stopping, the accept() loop will clear any pending sockets + if (active && !_active) + _synQueue.clear(); + boolean wasActive = _active; _active = active; - if (!active) { + if (wasActive && !active) { try { _synQueue.put(new PoisonPacket()); // so we break from the accept() - waits until space is available } catch (InterruptedException ie) {} } } + public boolean getActive() { return _active; } /** @@ -102,17 +112,21 @@ class ConnectionHandler { * * @param timeoutMs max amount of time to wait for a connection (if less * than 1ms, wait indefinitely) - * @return connection received, or null if there was a timeout or the - * handler was shut down + * @return connection received. Prior to 0.9.17, or null if there was a timeout or the + * handler was shut down. As of 0.9.17, never null. + * @throws ConnectException since 0.9.17, returned null before; + * if the I2PServerSocket is closed, or if interrupted. + * @throws SocketTimeoutException since 0.9.17, returned null before; + * if a timeout was previously set with setSoTimeout and the timeout has been reached. */ - public Connection accept(long timeoutMs) { + public Connection accept(long timeoutMs) throws ConnectException, SocketTimeoutException { if (_log.shouldLog(Log.DEBUG)) _log.debug("Accept("+ timeoutMs+") called"); long expiration = timeoutMs + _context.clock().now(); while (true) { if ( (timeoutMs > 0) && (expiration < _context.clock().now()) ) - return null; + throw new SocketTimeoutException("accept() timed out"); if (!_active) { // fail all the ones we had queued up while(true) { @@ -121,7 +135,7 @@ class ConnectionHandler { break; sendReset(packet); } - return null; + throw new ConnectException("ServerSocket closed"); } Packet syn = null; @@ -132,7 +146,11 @@ class ConnectionHandler { if (timeoutMs <= 0) { try { syn = _synQueue.take(); // waits forever - } catch (InterruptedException ie) { } // { break;} + } catch (InterruptedException ie) { + ConnectException ce = new ConnectException("Interrupted accept()"); + ce.initCause(ie); + throw ce; + } } else { long remaining = expiration - _context.clock().now(); // (dont think this applies anymore for LinkedBlockingQueue) @@ -144,14 +162,18 @@ class ConnectionHandler { break; try { syn = _synQueue.poll(remaining, TimeUnit.MILLISECONDS); // waits the specified time max - } catch (InterruptedException ie) { } + } catch (InterruptedException ie) { + ConnectException ce = new ConnectException("Interrupted accept()"); + ce.initCause(ie); + throw ce; + } break; } } if (syn != null) { if (syn.getOptionalDelay() == PoisonPacket.POISON_MAX_DELAY_REQUEST) - return null; + throw new ConnectException("ServerSocket closed"); // deal with forged / invalid syn packets in _manager.receiveConnection() diff --git a/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionManager.java b/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionManager.java index 196038eea6..de2eb86e25 100644 --- a/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionManager.java +++ b/apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionManager.java @@ -590,10 +590,14 @@ class ConnectionManager { /** * Something b0rked hard, so kill all of our connections without mercy. * Don't bother sending close packets. + * This will not close the ServerSocket. + * This will not kill the timer threads. * * CAN continue to use the manager. */ public void disconnectAllHard() { + //if (_log.shouldLog(Log.INFO)) + // _log.info("ConnMan hard disconnect", new Exception("I did it")); for (Iterator iter = _connectionByInboundId.values().iterator(); iter.hasNext(); ) { Connection con = iter.next(); con.disconnect(false, false); @@ -603,21 +607,30 @@ class ConnectionManager { _recentlyClosed.clear(); } _pendingPings.clear(); + // FIXME + // Ideally we would like to stop all TCBShare and all the timer threads here, + // but leave them ready to restart when things resume. + // However that's quite difficult. + // So the timer threads will continue to run. } /** * Kill all connections and the timers. * Don't bother sending close packets. + * As of 0.9.17, this will close the ServerSocket, killing one thread in accept(). * * CANNOT continue to use the manager or restart. * * @since 0.9.7 */ public void shutdown() { + //if (_log.shouldLog(Log.INFO)) + // _log.info("ConnMan shutdown", new Exception("I did it")); disconnectAllHard(); _tcbShare.stop(); _timer.stop(); _outboundQueue.close(); + _connectionHandler.setActive(false); } /** diff --git a/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PServerSocketFull.java b/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PServerSocketFull.java index c41c5082e8..f00e9039e0 100644 --- a/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PServerSocketFull.java +++ b/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PServerSocketFull.java @@ -1,5 +1,6 @@ package net.i2p.client.streaming.impl; +import java.net.ConnectException; import java.net.SocketTimeoutException; import net.i2p.I2PException; @@ -20,13 +21,20 @@ class I2PServerSocketFull implements I2PServerSocket { } /** - * Warning, unlike regular ServerSocket, may return null + * Waits for the next socket connecting. If a remote user tried to make a + * connection and the local application wasn't .accept()ing new connections, + * they should get refused (if .accept() doesnt occur in some small period). + * Warning - unlike regular ServerSocket, may return null (through 0.9.16 only). * - * @return I2PSocket OR NULL - * @throws net.i2p.I2PException - * @throws SocketTimeoutException + * @return a connected I2PSocket OR NULL through 0.9.16; never null as of 0.9.17 + * + * @throws I2PException if there is a problem with reading a new socket + * from the data available (e.g. the I2PSession is closed) + * @throws ConnectException if the I2PServerSocket is closed, or if interrupted. + * Not actually thrown through 0.9.16; thrown as of 0.9.17 + * @throws SocketTimeoutException if a timeout was previously set with setSoTimeout and the timeout has been reached. */ - public I2PSocket accept() throws I2PException, SocketTimeoutException { + public I2PSocket accept() throws I2PException, ConnectException, SocketTimeoutException { return _socketManager.receiveSocket(); } diff --git a/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PSocketManagerFull.java b/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PSocketManagerFull.java index 435207b6c3..6d948141a3 100644 --- a/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PSocketManagerFull.java +++ b/apps/streaming/java/src/net/i2p/client/streaming/impl/I2PSocketManagerFull.java @@ -1,6 +1,7 @@ package net.i2p.client.streaming.impl; import java.io.IOException; +import java.net.ConnectException; import java.net.NoRouteToHostException; import java.net.ServerSocket; import java.net.Socket; @@ -121,27 +122,19 @@ public class I2PSocketManagerFull implements I2PSocketManager { } /** + * The accept() call. * - * @return connected I2PSocket OR NULL - * @throws net.i2p.I2PException - * @throws java.net.SocketTimeoutException + * @return connected I2PSocket, or null through 0.9.16, non-null as of 0.9.17 + * @throws I2PException if session is closed + * @throws ConnectException (since 0.9.17; I2PServerSocket interface always declared it) + * @throws SocketTimeoutException if a timeout was previously set with setSoTimeout and the timeout has been reached. */ - public I2PSocket receiveSocket() throws I2PException, SocketTimeoutException { + public I2PSocket receiveSocket() throws I2PException, ConnectException, SocketTimeoutException { verifySession(); Connection con = _connectionManager.getConnectionHandler().accept(_connectionManager.getSoTimeout()); - if(_log.shouldLog(Log.DEBUG)) { - _log.debug("receiveSocket() called: " + con); - } - if (con != null) { - I2PSocketFull sock = new I2PSocketFull(con,_context); - con.setSocket(sock); - return sock; - } else { - if(_connectionManager.getSoTimeout() == -1) { - return null; - } - throw new SocketTimeoutException("I2PSocket timed out"); - } + I2PSocketFull sock = new I2PSocketFull(con, _context); + con.setSocket(sock); + return sock; } /** @@ -217,6 +210,13 @@ public class I2PSocketManagerFull implements I2PSocketManager { return _defaultOptions; } + /** + * Returns non-null socket. + * This method does not throw exceptions, but methods on the returned socket + * may throw exceptions if the socket or socket manager is closed. + * + * @return non-null + */ public I2PServerSocket getServerSocket() { _connectionManager.setAllowIncomingConnections(true); return _serverSocket; diff --git a/history.txt b/history.txt index bfb03b9ad0..414e5f39c5 100644 --- a/history.txt +++ b/history.txt @@ -1,3 +1,19 @@ +2014-11-13 zzz + * I2PTunnel: + - Fix bug that left server acceptor thread running after close + - Add destroy() methods to release all resources when closing a tunnel for good, + particularly the streaming timer threads + - Use COWAL to prevent concurrency problems + * PluginStarter: Simplify detection of active threads + * Streaming: + - Don't return null from accept() any more; actually throw + ConnectException as the Javadocs have always specified + - Throw ConnectException from accept() if interrupted; previously caught and ignored + - Throw exceptions from ConnectionHandler.accept(), not higher up + - Close ServerSocket when ConnectionManager is shut down + - Synchronize setActive(), clear queue when starting to accept, + better handling of calls that don't change state + 2014-11-12 zzz * Data: Clear more caches when under memory pressure and at shutdown * Plugins: Fix bug in stopping a ClientApp plugin with $parameters in the args diff --git a/router/java/src/net/i2p/router/RouterVersion.java b/router/java/src/net/i2p/router/RouterVersion.java index 4e28a2c8d2..c10128fe62 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 = 5; + public final static long BUILD = 6; /** for example "-test" */ public final static String EXTRA = "";