Opened 3 months ago

Last modified 11 days ago

#2700 new enhancement

Add option to restart tunnel after period of inactivity to Tunnel Manager client configuration

Reported by: Reportage Owned by:
Priority: minor Milestone: undecided
Component: apps/i2ptunnel Version: 0.9.45
Keywords: tunnel manager, destination cycling, latency Cc:
Parent Tickets: Sensitive: no

Description

In addition to the current option to close client tunnels after a period of inactivity, the option to restart a tunnel might be considered.

This would be useful in combination with the 'New keys on reopen' option to cycle destinations automatically while preventing the inherent latency in closing a tunnel and only restarting when activity is detected.

Subtickets

Attachments (3)

rekey.diff (64.7 KB) - added by idk 2 months ago.
Patch which implements the option as i2cp.restartOnIdle
restart.diff (12.0 KB) - added by idk 2 weeks ago.
New version, everything but the CSS, tested to work.
rekey.2.diff (12.1 KB) - added by idk 13 days ago.
A much simplified, timer-based version of tunnel restarts. This uses radiobuttons to avoid UI clutter and simply checks if a tunnel is closed, and re-opens it. It only applies to tunnels which have i2ptunnel;.newDestOnIdle=true, and the timer is not started until at least one socket has been opened, so unused tunnels will still simply close. That behavior is easy to change,

Download all attachments as: .zip

Change History (14)

Changed 2 months ago by idk

Attachment: rekey.diff added

Patch which implements the option as i2cp.restartOnIdle

comment:1 Changed 2 months ago by zzz

Another option suggestion I think would be rarely used. Need more info on the use case - who would use this?

But since we have a patch from idk, let's take a look…

  • the patch is hard to read because of all the unrelated whitespace changes, would be a lot easier on reviewers if you did not include whitespace changes in your diffs. Please fix your editor to not make whitespace changes.
  • I2CPMessageProcuder line 66 adds a second "i2cp.reduceIdleTime" element, looks wrong
  • This is a complex set of options and I don't know if the UI or the code really makes clear or handles the various combinations correctly. OP says only useful if new-keys-on-reopen is set. Also seems to be incompatible with close-on-idle. In the UI we have a radio to try to keep it all straight but this patch just adds a checkbox. Before we even get to looking at code carefully, would be nice to see a proposal on what option trumps what. Because this will eventually have to be documented.
  • For example, maybe it would be more straightforward to overload the existing closeOnIdle property, to be true, false, or "restart" ?
  • need @since on new methods
  • need spacing, "if (" not "if("
  • does disconnect and immediate reconnect work? no delay needed? is this tested?

tl;dr don't hate it but have almost zero patience for new option suggestions this year, we have no time. Patch is a good proof of concept (assuming it works? test results please?) but the interaction of all the options needs to be thought through

comment:2 Changed 2 months ago by zzz

Another issue, you're reusing the i2cp.closeIdleTime property for the restart time, but that defaults to 30 minutes, which seems way too short, should be more like 24 hours? I don't know if you'd want another property, or just use a different default, or what. There's UI implications here also.

comment:3 Changed 2 months ago by zzz

next problem: while i2cp.newDestOnResume looks like a i2cp option, it's actually implemented in I2PTunnelClientBase. You should confirm this by testing, but pretty sure that _session.disconnect() and _session.reconnect() won't give you a different dest. A session dest is final. So I don't think the patch is actually doing anything, you'd have to throw out that session and SocketManager? and make a new one. Double check but I think that the patch wouldn't work.

comment:4 Changed 2 months ago by zzz

i2cp.newDestOnResume was previously undocumented, added it to i2ptunnel config docs on the website
in 0eb92715c759ee7ae7741385b6c1234b99dcf889

comment:5 Changed 3 weeks ago by zzz

Version: 0.9.460.9.45

idk stated his intentions at the meeting this week to get this in for .46, but it's getting pretty late for that and I had major issues with the patch, see comments 1-3 above. This has real potential to break things. Request a new version of the patch and plenty of time for testing and review before anything gets checked in.

comment:6 Changed 3 weeks ago by idk

After several revisions I have a way of doing this of this that seems to work best for the HTTP Proxy, and probably will work for the rest of the client tunnels before the deadline. I also decided to scrap the complicated and much more invasive approach I was talking about before. The new way works like this:

Two new UI elements are added to the Tunnel Time Management table below "Close on Idle" related options. They are not settable unless "Close on Idle" is. These are "Resume on Idle" and and "Resume after Idle Time" a boolean and an integer representing a period of time. The description of these options indicate that their primary purpose is to cause idle tunnels to re-open to force a change of destination. The default "Resume after Idle Time" is 5 minutes chosen entirely arbitrarily in my current setup.

In I2PTunnelClientBase, a new private bool "wakeupTime" and an accessor are introduced. when a client tunnel is closed, a Timer is used to initiate a TimerTask?, upon completion of the TimerTask? wakeupTime is set to true, allowing the accessor to be used as a signal to classes that inherit from I2PClientTunnelBase. After the accessor returns true, the value of wakeupTime is reset to false.

In I2PTunnelHttpClient, when the tunnel is closed, the value of the "resumeIdle" property is checked. If the resumeIdle property is true, then the wakeupTime accessor is checked. If the wakeupTime accessor indicates that it is time to wake up, then an HTTP client is created, configured to use the I2PTunnelHttpClient as a proxy, and a request is made to proxy.i2p. This has the effect of waking the tunnel back up.

I need to see what exactly it takes to wake up standard clients in a similar way, but it's by far the simplest approach and it seems to be working. I'll add the patch as soon as I wake up a standard client tunnel without the service at the other end being able to know.

Last edited 3 weeks ago by idk (previous) (diff)

comment:7 Changed 2 weeks ago by zzz

It's a little difficult to follow the above, but I think I understand. You never confirmed whether the previous impl. was flawed - that it did not actually change the destination - nor is it clear that you're actually changing the destination with the new version.

It sounds like you're explaining how you're going to re-implement close-on-idle and new-dest-on-resume. That's already implemented, of course - it's in I2PTunnelClientBase together with I2PSessionImpl and SessionIdleTimer?. So I'm confused why you're describing an implementation for those.

I still don't understand why any code changes would be required to HTTPClient. There's nothing unique about this feature to the HTTP proxy, all code should be in the Base.

The new feature is new-dest-on-idle. The goal of the OP is not to simply "re-open an idle tunnel", but to _close and then reopen_ an idle tunnel, preferably in the middle of the night. You're not trying to "wake up" tunnels but to destroy them and create a new one.

As described in several of my comments above, I don't yet see how this is possible at all, issues include:

  • You don't have good (any?) visibility of idle time in i2ptunnel
  • Interaction with other shared clients, if any
  • Difficulties in the UI of how the settings will interact
  • Difficulties with doing timers to get it to happen at the right time (and no, don't use TimerTask?, use i2p timers)
  • General raciness, not sure it's avoidable

Would love to be proven wrong about all of it, will await the patch

Changed 2 weeks ago by idk

Attachment: restart.diff added

New version, everything but the CSS, tested to work.

comment:8 Changed 2 weeks ago by idk

I have to do the Javadoc and the CSS still, but it looks like it's not impossible! The old ones worked for some tunnels but not others, or didn't work reliably. Doesn't seem to be the case anymore.

The root of the confusion this morning seems to have been my fault, a) I had a false positive situation occuring because I was configuring the timer wrong, and b) I also didn't really characterize what I was doing with the timer very well. So I went over it in the daytime, switched it from an TimerTask? to a SimpleTimer2.TimedEvent?, fixed how the time was set, and spent some time testing it. What happens is that when activity occurs on the tunnel, an event is scheduled closeIdleTime+resumeIdleTime minutes from when the activity occurred. That event re-starts the tunnel. If more activity occurs, the event is then re-scheduled for the new restart time.

Last edited 2 weeks ago by idk (previous) (diff)

comment:9 Changed 2 weeks ago by zzz

major:

  • You're defining 'activity' as 'new socket opened on this client', not 'any traffic' (possibly on other shared clients). So you're not addressing my issues above, that i2ptunnel has poor visibility into idle time. This is why I still don't see how to do this at all. There is no 'signal' from I2CP to i2ptunnel that the tunnel closed. By design, i2ptunnel doesn't know.
  • Example: socket comes in, you set the timer for 31 minutes. Socket stays open for two minutes. 29 more minutes later, timer fires, session still open, timer schedules for another 31 minutes. 1 minute later, session closes on idle. Session stays closed for 30 minutes before timer fires again and you reopen. So the design goal is to open 1 minute later, but it was closed for 31 minutes.
  • I think there's a better way to do this without a separate timer, just do it in the accept() loop with a timeout there.
  • You have the closeOnIdle and resumeOnIdle boolean options as both being required for this feature, which is fine internally, but that's a mess in the UI where it should be a radio I think? To the user, it should be presented as one or the other, close or resume? I don't think the UI is worked out yet with the best way to present these options.

medium:

  • If resumeFromClose is just one minute for safety, we don't need it in the UI at all, just hardcode it. It would be way too confusing to put in the UI
  • Naming of the options seems wrong, it should be 'new dest', not 'resume' ? not sure
  • So shouldn't there be only one new option, a boolean 'i2cp.newDestOnIdle'?

minor:

  • spacing issues, tabs in there on a few lines
  • new java.io imports in Base, appear unrelated and unused
  • Log change in TCG is unrelated

comment:10 Changed 2 weeks ago by idk

I reliably reproduced the socket close/socket re-open race issue, learned a little, but clearly the way in that second patch is right-out. But I was thinking about it, and if the time between tunnel close and tunnel re-open is non-configurable(Not in line with my original intentions but I am adequately convinced that it should be this way for now) and meant to be brief, but non-zero(Which is in line with my original intentions, then it doesn't actually need to know when the tunnel is down or not, it just needs to check every minute or so and only take action if the tunnel isClosed.

ACK on the radio buttons vs/checkboxes. I will change it in the UI for the patch I submit tonight. I'll try to address the rest of your points as well. Thanks zzz.

Changed 13 days ago by idk

Attachment: rekey.2.diff added

A much simplified, timer-based version of tunnel restarts. This uses radiobuttons to avoid UI clutter and simply checks if a tunnel is closed, and re-opens it. It only applies to tunnels which have i2ptunnel;.newDestOnIdle=true, and the timer is not started until at least one socket has been opened, so unused tunnels will still simply close. That behavior is easy to change,

comment:11 Changed 11 days ago by zzz

I coded up my idea of doing it in the accept loop (no separate timer). See below. Untested. Just FYI, not proposing it as a real solution, I don't like it.

The enhancement it has over yours is it waits the full 30 minutes after a connection comes in - no use firing every minute until you're in the 'possibly idle phase'. (see comment 9 above).

However my solution and yours share the same flaw - waking up the thread every minute to poll the state. I don't think this is acceptable. The goal is to create some 'signal' that will get from I2CP thru streaming to i2ptunnel. That 'signal' doesn't exist now, which is why I keep saying this is hard, but I think it's the right way to do it.

Didn't review the UI part yet.

We agreed this won't make it in for 46, so we have time to get this right for a later release.

--- apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelClientBase.java	b4ad8a305ae6c2aa7e5af274828c61ccf830198e
+++ apps/i2ptunnel/java/src/net/i2p/i2ptunnel/I2PTunnelClientBase.java	20db2f0647f7f6a281ad98a31c332932d929e655
@@ -12,6 +12,7 @@ import java.net.Socket;
 import java.net.NoRouteToHostException;
 import java.net.ServerSocket;
 import java.net.Socket;
+import java.net.SocketTimeoutException;
 import java.net.UnknownHostException;
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -774,9 +775,36 @@ public abstract class I2PTunnelClientBas
                 // Never shut down.
                 _executor = new TunnelControllerGroup.CustomThreadPoolExecutor();
             }
+            boolean gotSocket = false;
             while (open) {
-                Socket s = ss.accept();
-                manageConnection(s);
+                int timeout;
+                if (Boolean.parseBoolean(getTunnel().getClientOptions().getProperty("i2ptunnel.resumeOnIdle"))) {
+                    if (gotSocket) {
+                        // can't go idle for at least this long
+                        try {
+                            timeout = Integer.parseInt(getTunnel().getClientOptions().getProperty("i2cp.closeIdleTime"));
+                            timeout += 60*1000;
+                        } catch(NumberFormatException ex){
+                            timeout = 31*60*1000;
+                        }
+                     } else {
+                        // we timed out, so now do the frequent check
+                        timeout = 60*1000;
+                     }
+                } else {
+                    // no new-dest-on-idle option, wait forever
+                    timeout = 0;
+                }
+                ss.setSoTimeout(timeout);
+                try {
+                    Socket s = ss.accept();
+                    gotSocket = true;
+                    manageConnection(s);
+                } catch (SocketTimeoutException ste) {
+                    gotSocket = false;
+                    if (Boolean.parseBoolean(getTunnel().getClientOptions().getProperty("i2ptunnel.resumeOnIdle")))
+                        verifySocketManager();
+                }
             }
         } catch (IOException ex) {
             synchronized (sockLock) {
Note: See TracTickets for help on using tickets.