Error in the ntcp reader: java.lang.RuntimeException

[guest]

10/07/13 05:30:26 CRIT [P reader 1/4] p.router.transport.ntcp.Reader: Error in the ntcp reader

java.lang.RuntimeException?: connection was not established, yet the establish state is null for NTCP conn from unknown not established created 187ms ago
at net.i2p.router.transport.ntcp.Reader.processRead(Reader.java:147)
at net.i2p.router.transport.ntcp.Reader.access$400(Reader.java:21)
at net.i2p.router.transport.ntcp.Reader$Runner.run(Reader.java:118)
at java.lang.Thread.run(Thread.java:722)
at net.i2p.util.I2PThread.run(I2PThread.java:85)

[guest]

I2P version: 0.9.6-0
Java version: Oracle Corporation 1.8.0-ea (Java(TM) SE Runtime Environment 1.8.0-ea-b36e)
Wrapper version: 3.5.17
Server version: 7.6.10.v20130312
Servlet version: Jasper JSP 2.1 Engine
Platform: Linux arm 3.6.11+
Processor: uninitialized (arm)
Jbigi: Locally optimized native BigInteger? library loaded from file
Encoding: UTF-8
Charset: UTF-8

[zzz]

This is some sort of really ugly race in NTCP. Needs research. Probably shouldn't be a CRIT.

[zab]

Here's a stab at this ​http://pastethis.i2p/show/4166/

[zzz]

I don't think that's sufficient, or perhaps even on the right track. Isn't the real problem in Reader.processRead() which is looping and checking multiple things (isClosed(), isEstablished(), getNextReadBuf()) - without any locking at all?

But back in NTCPConnection, _establishState isn't final and doesn't even get created in the outbound constructor - and so the "shouldn't happen" comment in prepareNextWriteFast() is wrong? Then the establishState gets nulled out after it's connected, which is fine I guess.

Just another example of crappy state management with several different fields and methods to get them.

Not clear why processRead() has to know about the establisher at all. I guess the idea is that if the handshakes are happening, the packets go the establisher, not to the connection itself via recvEncryptedI2NP().

Seems like a lot of it could be cleaned up by moving most of processRead() to NTCPConnection, and let NTCPConnection worry about its own state and routing incoming packets to either the estabisher or recvEncryptedI2NP, or throw/log/drop/etc. Then Reader doesn't have to use multiple non-locked NTCPConnection methods to determine the current state and try to do the right thing.

[zab]

I agree with all the points above. There is no single clean solution. Here is another try that incorporates some of the suggestions: ​http://pastethis.i2p/show/4172/

[zab]

And another stab, this time not holding on to the large EstablishSate? object ​http://pastethis.i2p/show/4174/

[zab]

Small bug in the Reader loop, fix: ​http://pastethis.i2p/show/4176/

[zab]

Ignore previous pastes, use ​http://pastethis.i2p/show/4178/

[zab]

After some suggestions from zzz ​http://pastethis.i2p/show/4179/

[zab]

Replying to zab:

After some suggestions from zzz ​http://pastethis.i2p/show/4179/

07:14:23:54:14:218 ERROR [NTCP Pumper ] ter.transport.ntcp.EventPumper: Error in the event pumper
java.lang.NullPointerException
        at net.i2p.router.transport.ntcp.EventPumper.runDelayedEvents(EventPumper.java:754)
        at net.i2p.router.transport.ntcp.EventPumper.run(EventPumper.java:170)
        at java.lang.Thread.run(Thread.java:722)
        at net.i2p.util.I2PThread.run(I2PThread.java:85)

[zab]

Replying to zab:
Addressed in ​http://pastethis.i2p/show/4182/

[zab]

Should be merged to trunk with revision 07a52fb185cd31462111fef287cc1ea1b7da09a4
version 0.9.7-4