Opened 5 years ago

Closed 5 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:
Keywords: Cc: zab@…
Parent Tickets:


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:



Change History (5)

comment:1 Changed 5 years ago by zab

  • Cc zab@… added

comment:2 Changed 5 years ago by zzz

  • Owner set to zzz
  • Status changed from new to accepted

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 5 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 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 5 years ago by zzz

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

comment:5 Changed 5 years ago by zzz

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

Propped in

Note: See TracTickets for help on using tickets.