Opened 23 months ago
Closed 23 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 23 months ago by
comment:2 Changed 23 months ago by
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 23 months ago by
Component: | router/general → streaming |
---|---|
Priority: | minor → major |
Type: | enhancement → defect |
comment:4 Changed 23 months ago by
Milestone: | undecided → 0.9.39 |
---|---|
Owner: | set to zzz |
Status: | new → accepted |
Summary: | Process ClientMessages on JobQueue instead of Internal Reader threads → Streaming 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 23 months ago by
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
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!
Update: after more testing I saw the warnings again. Investigating if the JobQueue gets backed up.