UDP EstablishmentManager tries to resend expired messages

[jogger]

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.

[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.

[zzz]

Above patch in 7ff58573c1cce2cd56e7bb541af6d5f8183bc975 to be 0.9.47-5

leaving ticket open to await clarification on the other points

[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.