Opened 6 months ago

Closed 6 months ago

#2451 closed defect (fixed)

Streaming sets I2CP expiration in the past

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

Description

With logging enabled at WARN level I was observing frequent "Expired before we got to it" messages and failure code 14. Then I changed router/java/src/net/i2p/router/ClientMessagePool.java to de-inline the processing of OCMOSJ and hand them off to the JobQueue.

The warnings disappeared and browsing with 10 outproxies "feels" snappier, fwiw.

Subtickets

Change History (5)

comment:1 Changed 6 months ago by Zlatin Balevsky

Update: after more testing I saw the warnings again. Investigating if the JobQueue gets backed up.

comment:2 Changed 6 months ago by Zlatin Balevsky

Found the issue, has nothing to do with JobQueue or overload. The streaming lib reduces the timeout of the message by 500ms before it sends it to the I2CP layer, but it doesn't check if the new expiration date is in the past. The following patch fixes this:

#
# old_revision [7ad38b17696cea2dee16e912d982b44adb252ef0]
#
# patch "apps/streaming/java/src/net/i2p/client/streaming/impl/PacketQueue.java"
#  from [a88ba8984c8117813d56c36751dc4349eeb799aa]
#    to [10d97928a9c7e3399e4147d47635c368f77ad296]
#
============================================================
--- apps/streaming/java/src/net/i2p/client/streaming/impl/PacketQueue.java	a88ba8984c8117813d56c36751dc4349eeb799aa
+++ apps/streaming/java/src/net/i2p/client/streaming/impl/PacketQueue.java	10d97928a9c7e3399e4147d47635c368f77ad296
@@ -117,10 +117,12 @@ class PacketQueue implements SendMessage
             begin = _context.clock().now();
             long expires = 0;
             Connection.ResendPacketEvent rpe = (Connection.ResendPacketEvent) packet.getResendEvent();
-            if (rpe != null)
+            if (rpe != null) {
                 // we want the router to expire it a little before we do,
                 // so if we retransmit it will use a new tunnel/lease combo
                 expires = rpe.getNextSendTime() - 500;
+                expires = Math.max(expires, begin + 10);
+            }
             SendMessageOptions options = new SendMessageOptions();
             if (expires > 0)
                 options.setDate(expires);

comment:3 Changed 6 months ago by Zlatin Balevsky

Component: router/generalstreaming
Priority: minormajor
Type: enhancementdefect

comment:4 Changed 6 months ago by zzz

Milestone: undecided0.9.39
Owner: set to zzz
Status: newaccepted
Summary: Process ClientMessages on JobQueue instead of Internal Reader threadsStreaming sets I2CP expiration in the past

Yeah, great catch. The minus 500 line is from 2009, but back then the min RTO was 2000, now it's 100.
500 is way too big but I also want to explore if we still want to do it at all. If a retx is coming do we actually want to drop it early on the router side or not? If we are going to do the math.max, not sure +10 is enough. Let's think about the right way to do this.

comment:5 Changed 6 months ago by zzz

Resolution: fixed
Status: acceptedclosed

Reduced from 500 to 25, and parameterized it so we won't make this mistake again.
Increased the adder from 10 to 25, to account for out-of-jvm clients.
In dfc576d8521d615d5c37e7ed5cc7b3962ff00e09 0.9.38-12
Great job!

Note: See TracTickets for help on using tickets.