Opened 6 years ago

Closed 6 years ago

#1039 closed defect (fixed)

Bad tracking of active connections

Reported by: zzz Owned by: zzz
Priority: minor Milestone: 0.9.9
Component: streaming Version: 0.9.7.1
Keywords: Cc: zab@…
Parent Tickets: Sensitive: no

Description

Connection.getIsConnected() returns true long after a close has been received and sent. It doesn't return false until disconnect() is called.

This results in ConnectionManager?.locked_tooManyStreams() returning true and rejecting connections unnecessarily. This hurts availability on busy servers.

Possible solution: check for close sent and close received instead. Beware of half-close issues, and state tracking problems in Connection. See successive tickets.

annotated logs:

http://pastethis.i2p/show/wIHnFl1IEkhSdhxKxWdH/

Subtickets

Change History (5)

comment:1 Changed 6 years ago by Zlatin Balevsky

Cc: zab@… added

comment:2 Changed 6 years ago by zzz

Owner: set to zzz
Status: newaccepted

related: #1040 #1041 #1042 #1043

The count currently includes all open conns and all conns closed in the last 5 minutes. Clearly this is wrong for everybody and could be very wrong on a busy server.

proposed interim fix: http://pastethis.i2p/show/eNeHH1P6e7exhuSeVZE2/

"interim" because work on the other tickets may require updates to this fix.

comment:3 Changed 6 years ago by zzz

<zab> re patch for #1039 :
<zab> if a close has either only been sent or only been received, do we want to count that connection as active?
<zzz> zab it's almost a don't care now, since sent and rcvd will be almost simultaneous at the client, and one RTT apart at the server

<zzz> however if we start dabbling with SYN+CLOSE, we would want && not

<zzz> so it's a little bit forward looking to use &&
<zzz> thats my thinking
<zab> ok. Do we want to return true if active == max?
<zab> I see that's the current behavior, just asking
<zzz> yes thats what it did before
<zzz> I may have a fencepost error in my patch, didnt stare at it too hard
<zab> I think it's fine
<zab> ≥ active and > inactive
<zab> another question: why not move the getClose{Sent,Recv} inside the getIsConnected() method?
<zzz> i wanted to kick out of that loop asap because it's kinda O(n2) once you go over max
<zzz> re: getClose(), I didn't want to get into modifying Connection.java or researching all the uses for getIsConnected()
<zzz> by changing locked_tooManyStreams() only, it's provably non-disruptive / low-risk
<zzz> I consider this an "interim" fix as later changes to Connection to fix the other problems will probably allow an improvement in locked_tooManySTreams() later
<zab> ok. Are you going to push it now or wait after 0.9.8 is out?
<zzz> wait
<zab> are you running your eepsites with this fix?
<zab> I'm asking if we've seen it reduce the load on real-world eepsites
<zzz> no. untested.

comment:4 Changed 6 years ago by zzz

Patch above checked in to i2p.i2p.zzz.test2 555dd869c3af4c18bfb9ede8151f2ffb40cea5c2 to be propped for 0.9.9

comment:5 Changed 6 years ago by zzz

Resolution: fixed
Status: acceptedclosed

Propped in 0.9.8.1-2

Note: See TracTickets for help on using tickets.