EventPumper.java: High CPU, leaks memory

[jogger]

The bug is around runDelayedEvents() looping over _wantsWrite, when closed connections with null _conKey get stuck there forever.

The failsafe code does not catch this. As a quick fix one could loop over _wantsWrite during failsafe to remove closed cons or replace line 821 with

                if ((con = iter.next()).isClosed()) {
                    iter.remove();
                    continue;
                };

Also a list would be more efficient than a hash set. Frequently modified hash maps grow indefinitely large over time even when nearly empty and should be disposed of reqularly.

[zzz]

How about this? Testing now.

--- router/java/src/net/i2p/router/transport/ntcp/EventPumper.java	26ab2df00fcedcf6ff068a4773bf7a1246c5be49
+++ router/java/src/net/i2p/router/transport/ntcp/EventPumper.java	78f54c3cae6141714567bb296fd7bd75784c9298
@@ -819,10 +819,12 @@ class EventPumper implements Runnable {
         if (!_wantsWrite.isEmpty()) {
             for (Iterator<NTCPConnection> iter = _wantsWrite.iterator(); iter.hasNext(); ) {
                 con = iter.next();
+                iter.remove();
+                if (con.isClosed())
+                    continue;
                 SelectionKey key = con.getKey();
                 if (key == null)
                     continue;
-                iter.remove();
                 try {
                     key.interestOps(key.interestOps() | SelectionKey.OP_WRITE);
                 } catch (CancelledKeyException cke) {

[jogger]

I have not fully gone through all possible cases. If the (key == null) test is there because of a pending write waiting for the conkey to be set then the patches act differently. Otherwise yours is better of course.

[jogger]

Sorry for the short answer. I coded the patch because of this found in router/java/src/net/i2p/router/transport/ntcp/NTCPConnection.java.

The connections found in the heap dump were outbound.

     *  Valid for inbound; valid for outbound shortly after creation
     */
    public synchronized SelectionKey getKey() { return _conKey; }

[zzz]

isClosed() is probably sufficient but to be safe, left the null check in there too.
But I don't see how the key could be null, wantsWrite() shouldn't be called before the connection happens and the key is set.

In e1bf50e69f3df6c9fca5547052ea6d73b692246f 0.9.44-8-rc