Opened 6 years ago

Closed 6 years ago

#972 closed defect (fixed)

Error in the ntcp reader: java.lang.RuntimeException

Reported by: guest Owned by: zzz
Priority: minor Milestone: 0.9.9
Component: router/transport Version: 0.9.6
Keywords: Cc: zab@…
Parent Tickets:


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(
at net.i2p.router.transport.ntcp.Reader.access$400(
at net.i2p.router.transport.ntcp.Reader$


Change History (12)

comment:1 Changed 6 years ago by 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

comment:2 Changed 6 years ago by zzz

  • Component changed from unspecified to router/transport
  • Milestone changed from 0.9.7 to 0.9.9
  • Owner set to zzz

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

comment:3 Changed 6 years ago by zab

  • Cc zab@… added

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

comment:4 Changed 6 years ago by 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.

comment:5 Changed 6 years ago by 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/

comment:6 Changed 6 years ago by zab

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

comment:7 Changed 6 years ago by zab

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

comment:8 Changed 6 years ago by zab

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

comment:9 follow-up: Changed 6 years ago by zab

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

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by 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
        at net.i2p.router.transport.ntcp.EventPumper.runDelayedEvents(

comment:11 in reply to: ↑ 10 Changed 6 years ago by zab

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

comment:12 Changed 6 years ago by zab

  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.