Opened 10 months ago

Last modified 10 months ago

#2445 new enhancement

Make streaming compliant with RFC 6298

Reported by: Zlatin Balevsky Owned by:
Priority: maintenance Milestone: undecided
Component: streaming Version: 0.9.38
Keywords: Cc:
Parent Tickets: Sensitive: no

Description

Some of the constants used in the streaming lib need to be tweaked to be compliant with RFC 6298. See attached patch

#
# old_revision [c89bc469fa309546ca7532cd3908a172284e5df4]
#
# patch "apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java"
#  from [19ecf6a84c5c553a6456e13e424dbbebf39260d0]
#    to [e6ab30e8f188c6350eeac774a6d02189e7231678]
# 
# patch "apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java"
#  from [ee057b3e426d63bf9e6884eb2a0f3c9ffe122aec]
#    to [55901c775a6c7de753822de70e2f0c8771f79cd4]
#
============================================================
--- apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java	19ecf6a84c5c553a6456e13e424dbbebf39260d0
+++ apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java	e6ab30e8f188c6350eeac774a6d02189e7231678
@@ -96,8 +96,8 @@ class Connection {
     private final AtomicLong _lifetimeDupMessageSent = new AtomicLong();
     private final AtomicLong _lifetimeDupMessageReceived = new AtomicLong();
     
-    public static final long MAX_RESEND_DELAY = 45*1000;
-    public static final long MIN_RESEND_DELAY = 100;
+    public static final long MAX_RESEND_DELAY = 60*1000;
+    public static final long MIN_RESEND_DELAY = 1000;
 
     /**
      *  Wait up to 5 minutes after disconnection so we can ack/close packets.
============================================================
--- apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java	ee057b3e426d63bf9e6884eb2a0f3c9ffe122aec
+++ apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java	55901c775a6c7de753822de70e2f0c8771f79cd4
@@ -88,7 +88,7 @@ class ConnectionOptions extends I2PSocke
     private static final double TCP_KAPPA = 4;
     
     private static final String PROP_INITIAL_RTO = "i2p.streaming.initialRTO";
-    private static final int INITIAL_RTO = 9000; 
+    private static final int INITIAL_RTO = 1000; 
     
     public static final String PROP_CONNECT_DELAY = "i2p.streaming.connectDelay";
     public static final String PROP_PROFILE = "i2p.streaming.profile"

Subtickets

Change History (11)

comment:1 Changed 10 months ago by Zlatin Balevsky

Priority: majormaintenance

comment:2 Changed 10 months ago by zzz

I support reviewing recent RFCs and our current defaults. Of course typical end-to-end RTTs and losses over I2P are a lot higher than clearnet. Any adjustments should be based on a review of the history to see what we've done before, and why. And on a review of stats.

In particular, INITIAL_RTO may be too high at 9000, but 1000 is way too low, based on experience and current stats. On one router I have:

stream.con.initialRTT.out: 1532
stream.con.initialRTT.in: 2661

And my stats may be better than for most people. Our first RTT times are large due to conn setup time, especially the OBEP-IBGW connections. Things may be getting better due to transport improvements, so it is worth reviewing these settings periodically. But 1000 would be a disaster, we'd be retransmitting everything. No matter what the RFC says.

comment:3 Changed 10 months ago by Zlatin Balevsky

There is one interesting detail in the RFC:

   This document reduces the initial RTO from the previous 3 seconds
   [PA00] to 1 second, unless the SYN or the ACK of the SYN is lost, in
   which case the default RTO is reverted to 3 seconds before data
   transmission begins.

With a more detailed discussion in the appendix https://tools.ietf.org/html/rfc6298#appendix-A .

     When this happens, this document calls for reverting to an initial
     RTO of 3 seconds for the data transmission phase.  Therefore, the
     implications of the spurious retransmission are modest: (1) an
     extra SYN is transmitted into the network, and (2) according to RFC
     5681 [APB09] the initial congestion window will be limited to 1
     segment.  While (2) clearly puts such connections at a
     disadvantage, this document at least resets the RTO such that the
     connection will not continually run into problems with a short
     timeout.  (Of course, if the RTT is more than 3 seconds, the
     connection will still encounter difficulties.  But that is not a
     new issue for TCP.)

My take on this is that if 1 second is enough for plain TCP links, and we need to establish two OBEP-IBGW links per streaming connection, then 2 or 3 seconds should be enough. As the RFC recommends falling back to a 3-second RTO should the initial SYN or it's ACK timeout, then we can fall back on 9 seconds for the data phase. (This is of course a more complicated change as some logic needs to be updated).

comment:4 Changed 10 months ago by Zlatin Balevsky

A patch that implements section 5.7 of the RFC with the proposed values from comment 3:

#
# old_revision [82261f8cce1789f65a35fde8a287515d97f888d3]
#
# patch "apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java"
#  from [19ecf6a84c5c553a6456e13e424dbbebf39260d0]
#    to [e6ab30e8f188c6350eeac774a6d02189e7231678]
# 
# patch "apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java"
#  from [ee057b3e426d63bf9e6884eb2a0f3c9ffe122aec]
#    to [bc50d023a5311f6276107f344ca74f091f9c38f3]
#
============================================================
--- apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java	19ecf6a84c5c553a6456e13e424dbbebf39260d0
+++ apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java	e6ab30e8f188c6350eeac774a6d02189e7231678
@@ -96,8 +96,8 @@ class Connection {
     private final AtomicLong _lifetimeDupMessageSent = new AtomicLong();
     private final AtomicLong _lifetimeDupMessageReceived = new AtomicLong();
     
-    public static final long MAX_RESEND_DELAY = 45*1000;
-    public static final long MIN_RESEND_DELAY = 100;
+    public static final long MAX_RESEND_DELAY = 60*1000;
+    public static final long MIN_RESEND_DELAY = 1000;
 
     /**
      *  Wait up to 5 minutes after disconnection so we can ack/close packets.
============================================================
--- apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java	ee057b3e426d63bf9e6884eb2a0f3c9ffe122aec
+++ apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java	bc50d023a5311f6276107f344ca74f091f9c38f3
@@ -55,6 +55,7 @@ class ConnectionOptions extends I2PSocke
     private String _limitAction;
     private int _tagsToSend;
     private int _tagThreshold;
+    private boolean _rtoTripled;
     
     /** state of a connection */
     private enum AckInit {
@@ -88,7 +89,7 @@ class ConnectionOptions extends I2PSocke
     private static final double TCP_KAPPA = 4;
     
     private static final String PROP_INITIAL_RTO = "i2p.streaming.initialRTO";
-    private static final int INITIAL_RTO = 9000; 
+    private static final int INITIAL_RTO = 3000; 
     
     public static final String PROP_CONNECT_DELAY = "i2p.streaming.connectDelay";
     public static final String PROP_PROFILE = "i2p.streaming.profile";
@@ -645,8 +646,11 @@ class ConnectionOptions extends I2PSocke
      * @since 0.9.33
      */
     synchronized void doubleRTO() {
-        // we don't need to switch on _initState, _rto is set in constructor
-        _rto *= 2;
+        if (_initState == AckInit.INIT && !_rtoTripled) {
+	    _rto *= 3; // section 5.7 RFC 6298
+            _rtoTripled = true;
+        } else
+            _rto *= 2;
         if (_rto > Connection.MAX_RESEND_DELAY)
             _rto = (int)Connection.MAX_RESEND_DELAY;
Last edited 10 months ago by Zlatin Balevsky (previous) (diff)

comment:5 Changed 10 months ago by zzz

Interesting. Unfortunately, the situation is a lot messier than even the revised patch addresses.

The RFC says 'NBD, so we retx a SYN at 1 second, not the worst thing'. But TCP SYNs are small. We can bundle initial data, and (e.g. for POSTs and for responses) send another 5 packets along with the SYN, up to 6*1730 bytes, or a little more (the dest in the SYN, overhead at lower layers, etc.). RFC talks about "data transmission phase", we are always in that phase.

And retransmissions for all the packets are governed by their own retx timers (doubling each time), so changing the RTO after the first send won't affect the retx schedule for them.

Seems like the only way we could do it is to divide by 3 on initial success, rather than multiplying by 3 on initial failure. But even then, it wouldn't affect the retx schedule for unacked packets in flight. Perhaps we could ignore this issue if it's too hard to solve, but then you have the later sequence numbers could be retx'ed before the earlier ones, which is wasteful at best.

None of these issues are relevant to the related #2443.

comment:6 Changed 10 months ago by Zlatin Balevsky

Hmm. So other than giving up on the idea I see two options:

a) make the retx timers for packets in flight adjustable. This would be a non-trivial code change to say the least
b) stop sending data on the back of the initial SYN, i.e. become more like TCP. This will also be a serious code change AND will have visible impact on user experience of browsing eepsites and such. But will it necessarily be a bad impact overall?

comment:7 Changed 10 months ago by Zlatin Balevsky

Actually we only allow data to piggyback on the SYN if the i2p.streaming.connectDelay is greater than 0. Here is a modified patch which only applies the logic from the RFC if connectDelay < 0 and the user did not provide a custom RTO:

#
# old_revision [c89bc469fa309546ca7532cd3908a172284e5df4]
#
# patch "apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java"
#  from [19ecf6a84c5c553a6456e13e424dbbebf39260d0]
#    to [e6ab30e8f188c6350eeac774a6d02189e7231678]
# 
# patch "apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java"
#  from [ee057b3e426d63bf9e6884eb2a0f3c9ffe122aec]
#    to [276e2758581e375157f756f115d94c27849ad987]
#
============================================================
--- apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java	19ecf6a84c5c553a6456e13e424dbbebf39260d0
+++ apps/streaming/java/src/net/i2p/client/streaming/impl/Connection.java	e6ab30e8f188c6350eeac774a6d02189e7231678
@@ -96,8 +96,8 @@ class Connection {
     private final AtomicLong _lifetimeDupMessageSent = new AtomicLong();
     private final AtomicLong _lifetimeDupMessageReceived = new AtomicLong();
     
-    public static final long MAX_RESEND_DELAY = 45*1000;
-    public static final long MIN_RESEND_DELAY = 100;
+    public static final long MAX_RESEND_DELAY = 60*1000;
+    public static final long MIN_RESEND_DELAY = 1000;
 
     /**
      *  Wait up to 5 minutes after disconnection so we can ack/close packets.
============================================================
--- apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java	ee057b3e426d63bf9e6884eb2a0f3c9ffe122aec
+++ apps/streaming/java/src/net/i2p/client/streaming/impl/ConnectionOptions.java	276e2758581e375157f756f115d94c27849ad987
@@ -55,6 +55,7 @@ class ConnectionOptions extends I2PSocke
     private String _limitAction;
     private int _tagsToSend;
     private int _tagThreshold;
+    private boolean _rtoTripled;
     
     /** state of a connection */
     private enum AckInit {
@@ -88,7 +89,7 @@ class ConnectionOptions extends I2PSocke
     private static final double TCP_KAPPA = 4;
     
     private static final String PROP_INITIAL_RTO = "i2p.streaming.initialRTO";
-    private static final int INITIAL_RTO = 9000; 
+    private static final int INITIAL_RTO = 3000; 
     
     public static final String PROP_CONNECT_DELAY = "i2p.streaming.connectDelay";
     public static final String PROP_PROFILE = "i2p.streaming.profile";
@@ -408,10 +409,21 @@ class ConnectionOptions extends I2PSocke
         else
             _limitAction = DEFAULT_LIMIT_ACTION;
         
-        _rto = getInt(opts, PROP_INITIAL_RTO, INITIAL_RTO);
+        initRTO(opts);	
         _tagsToSend = getInt(opts, PROP_TAGS_TO_SEND, DEFAULT_TAGS_TO_SEND);
         _tagsToSend = getInt(opts, PROP_TAG_THRESHOLD, DEFAULT_TAG_THRESHOLD);
     }
+
+    private void initRTO(Properties opts) {
+	_rto = getInt(opts, PROP_INITIAL_RTO, INITIAL_RTO);
+        String override = opts.getProperty(PROP_INITIAL_RTO);
+        if (override != null) {
+            _rtoTripled = true; // assume user knows what they want
+	} else if ( getConnectDelay() > 0) {
+            _rtoTripled = true;
+	    _rto *= 3;
+        }
+    }
     
     /**
      *  Note: NOT part of the interface
@@ -484,8 +496,8 @@ class ConnectionOptions extends I2PSocke
             _maxConns = getInt(opts, PROP_TAGS_TO_SEND, DEFAULT_TAGS_TO_SEND);
         if (opts.getProperty(PROP_TAG_THRESHOLD) != null)
             _maxConns = getInt(opts, PROP_TAG_THRESHOLD, DEFAULT_TAG_THRESHOLD);
-        
-        _rto = getInt(opts, PROP_INITIAL_RTO, INITIAL_RTO);
+    
+        initRTO(opts);	
     }
     
     /** 
@@ -645,8 +657,11 @@ class ConnectionOptions extends I2PSocke
      * @since 0.9.33
      */
     synchronized void doubleRTO() {
-        // we don't need to switch on _initState, _rto is set in constructor
-        _rto *= 2;
+        if (_initState == AckInit.INIT && !_rtoTripled) {
+	    _rto *= 3; // section 5.7 RFC 6298
+            _rtoTripled = true;
+        } else
+            _rto *= 2;
         if (_rto > Connection.MAX_RESEND_DELAY)
             _rto = (int)Connection.MAX_RESEND_DELAY;
     }

comment:8 Changed 10 months ago by zzz

I don't think a) or b) is viable.

a) would be a mess and doesn't sound like a good idea
b) would revert one of the big advantages of streaming and part of its original design.

We try very hard to send initial data with the SYN; for an HTTP GET, the full request generally fits in the SYN. While the connect delay option defaults to -1, we set it to 1000 in the HTTP client tunnel, and then handle both the GET and POST cases in I2PTunnelRunner to get all the data in there and then flush() to minimize latency.

Even without initial data in the SYN, the SYN ends up being several KB due to the destination, reply leaseset, session tags, garlic overhead, etc., so removing the initial data alone doesn't make it tiny anyway.

So the patch above would only reduce the RTO for the HTTP server side and standard tunnels, where connectdelay is -1.

Not sure if making the decision based on connect delay is correct. Will think about it some more.

comment:9 Changed 10 months ago by zzz

INITIAL_RTO History:

  • Reduced from 12000 to 9000 in July 2013 and other major changes (add mini state machine)
  • Major changes in March 2007

I think we could probably reduce the default for the server-side, in i2ptunnel. On the server side, you already have the LS, and the inbound path via OBEP-IBGW is already set up, so in general a server should see lower initial RTTs than a client. Unfortunately, that's the opposite of the data I posted in comment 2.

Subject to further research and resolution of that, it may be appropriate to set a RTO default of 6000-7500 in i2ptunnel for servers.

comment:10 Changed 10 months ago by Zlatin Balevsky

Subject to further research and resolution of that, it may be appropriate to set a RTO default of 6000-7500 in i2ptunnel for servers.

Well that no longer matches the intent of this ticket, and does not help applications other than i2ptunnel, like snark. If that is the decision, you can close this ticket as wontfix.

comment:11 Changed 10 months ago by zzz

no no, haven't given up yet, just thinking through the broader issues

Note: See TracTickets for help on using tickets.