Opened 7 weeks ago

Closed 7 weeks ago

#2686 closed defect (fixed)

EventPumper.java: High CPU, leaks memory

Reported by: jogger Owned by: zzz
Priority: major Milestone: 0.9.45
Component: router/transport Version: 0.9.44
Keywords: Cc:
Parent Tickets: Sensitive: no

Description

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.

Subtickets

Change History (4)

comment:1 Changed 7 weeks ago by zzz

Milestone: undecided0.9.45
Status: newaccepted

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) {

`

Version 0, edited 7 weeks ago by zzz (next)

comment:2 Changed 7 weeks ago by 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.

comment:3 Changed 7 weeks ago by 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; }

comment:4 Changed 7 weeks ago by zzz

Resolution: fixed
Status: acceptedclosed

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

Note: See TracTickets for help on using tickets.