Opened 7 months ago

Last modified 6 months ago

#2771 new defect

UDP EstablishmentManager tries to resend expired messages

Reported by: jogger Owned by: zzz
Priority: minor Milestone: undecided
Component: router/transport Version: 0.9.47
Keywords: Cc:
Parent Tickets: Sensitive: no

Description

processExpired() calls failed(), which in turn goes to afterSend(). There the message gets requeued, is found as invalid in OutNetMessagePool?.add() and finally arrives at OutboundMessageRegistry?.unregisterPending() with a null reply selector.

It is clear that add() misses the reply selector null check that is always prepended to unregisterPending() elsewhere.

However I think the message should not be resent at all. Furthermore afterSend() checks message expiration which obviously gives a different result than the term "expired
(rtime > 0 && rtime + OB_MESSAGE_TIMEOUT ⇐ now)" used in handleOutbound().

To verify throw NPE in unregisterPending() for a null reply selector and wait hours/days.

Subtickets

Change History (3)

comment:1 Changed 7 months ago by zzz

Yeah calling unregisterPending() with a null reply selector appears to be harmless, but would be better to not do it, here's a diff for that:

#
# old_revision [775b2357d06cb63fd067caea3f192c10655abf6b]
#
# patch "router/java/src/net/i2p/router/OutNetMessagePool.java"
#  from [0e1696fa52158338f5f33114c122059d09d8d95b]
#    to [71cabb06ac294faef1db4f7b5e293a888182e5e7]
#
============================================================
--- router/java/src/net/i2p/router/OutNetMessagePool.java	0e1696fa52158338f5f33114c122059d09d8d95b
+++ router/java/src/net/i2p/router/OutNetMessagePool.java	71cabb06ac294faef1db4f7b5e293a888182e5e7
@@ -33,16 +33,18 @@ public class OutNetMessagePool {
      *
      */
     public void add(OutNetMessage msg) {
+        if (msg == null) return;
+        MessageSelector selector = msg.getReplySelector();
         boolean valid = validate(msg);
         if (!valid) {
-            _context.messageRegistry().unregisterPending(msg);
+            if (selector != null)
+                _context.messageRegistry().unregisterPending(msg);
             return;
         }        
         
         if (_log.shouldLog(Log.DEBUG))
             _log.debug("Adding " + msg);
         
-        MessageSelector selector = msg.getReplySelector();
         if (selector != null) {
             _context.messageRegistry().registerPending(msg);
         }
@@ -51,7 +53,6 @@ public class OutNetMessagePool {
     }
     
     private boolean validate(OutNetMessage msg) {
-        if (msg == null) return false;
         if (msg.getMessage() == null) {
             if (_log.shouldLog(Log.WARN))
                 _log.warn("Null message in the OutNetMessage - expired too soon");

The idea is that a timeout for an individual transport could be less than the overall message expiration, so it could be resent by another transport. But if the overall message has expired, we shouldn't. Not sure I understand what OP is proposing in the 3rd paragraph.

Last edited 7 months ago by zzz (previous) (diff)

comment:2 Changed 7 months ago by zzz

Above patch in 7ff58573c1cce2cd56e7bb541af6d5f8183bc975 to be 0.9.47-5

leaving ticket open to await clarification on the other points

comment:3 Changed 6 months ago by jogger

In Paragraph 3 I wanted to express that it makes little sense to retry a connection that already failed while establishing. So I would imagine a shortcut somewhere to drop everything related instead of calling methods aimed at retransmitting on working connections.

Note: See TracTickets for help on using tickets.