Opened 3 months ago

Closed 3 months ago

#2709 closed defect (fixed)

Streaming: Second retransmit happens after 4xRTO

Reported by: Zlatin Balevsky Owned by: zzz
Priority: major Milestone: 0.9.46
Component: streaming Version: 0.9.45
Keywords: Cc:
Parent Tickets: Sensitive: no

Description

In Connection.java, line 1538 the rto is doubled via a call to

getOptions().doubleRTO()

And that is correct. However, further down in line 1584 the *already doubled* RTO is shifted left by at least 1, i.e. multiplied by 2. As a result the second retransmission of the packet happens after 4xRTO, not 2x. All subsequent retransmissions are also twice as delayed.

I propose we change the shift to subtract 2 instead of 1:

-- a/apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java
+++ b/apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java
@@ -1581,7 +1581,7 @@ class Connection {
                     long rto = _options.getRTO();
                     if (rto < MIN_RESEND_DELAY)
                         rto = MIN_RESEND_DELAY;
-                    long timeout = rto << (numSends-1);
+                    long timeout = rto << (numSends-2); 
                     if ( (timeout > MAX_RESEND_DELAY) || (timeout <= 0) )
                         timeout = MAX_RESEND_DELAY;
                     // set this before enqueue() as it passes it on to the router

Subtickets

Change History (4)

comment:1 Changed 3 months ago by zzz

Milestone: undecided0.9.46
Owner: set to zzz
Priority: minormajor
Status: newaccepted
Version: 0.9.460.9.45

Confirmed in testing, transmissions are at 0/9/45 seconds instead of intended 0/9/27.
Appears to have happened Dec. 2017 in fix for #1939
Comments at line 1538 indicate what we were trying to do here.
Will need to ensure that the above fix doesn't break the #1939 fix.

comment:2 Changed 3 months ago by zzz

Add a subticket #2715 (Streaming: single retransmit timer per connection).

comment:3 Changed 3 months ago by zzz

Remove a subticket #2715 (Streaming: single retransmit timer per connection).

comment:4 Changed 3 months ago by zzz

Resolution: fixed
Status: acceptedclosed

Tested and reviewed here, looks good.
Fixed in 69648178070f14ca7258f3b1e0aaa19a3ccf812f 0.9.45-8 along with some minor cleanups and prep for #2715
#2715 also fixed this but wanted to get this standalone change in there first while we review #2715 and related tickets in detail.

Note: See TracTickets for help on using tickets.