Opened 7 years ago
Closed 7 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: | Zlatin Balevsky | |
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:
Subtickets
Change History (5)
comment:1 Changed 7 years ago by
Cc: | Zlatin Balevsky added |
---|
comment:2 Changed 7 years ago by
Owner: | set to zzz |
---|---|
Status: | new → accepted |
comment:3 Changed 7 years ago by
<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> 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 7 years ago by
Patch above checked in to i2p.i2p.zzz.test2 555dd869c3af4c18bfb9ede8151f2ffb40cea5c2 to be propped for 0.9.9
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.