Opened 9 months ago

Last modified 3 months 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 (7)

rekey.diff (64.7 KB) - added by idk 8 months ago.
Patch which implements the option as i2cp.restartOnIdle
restart.diff (12.0 KB) - added by idk 7 months ago.
New version, everything but the CSS, tested to work.
rekey.2.diff (12.1 KB) - added by idk 7 months 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,
resume.diff (17.0 KB) - added by idk 4 months ago.
This version uses the existing timer which does reduceOnIdle and closeOnIdle timer to do the whole disconnect, re-key, re-open sequence all at once. However, in order to do it, the _myDestination must not be final. The advantage is that it co-exists just fine with the existing timers, and does not require a second timer. It's also tested and has the expected effect of changing the destination of the client tunnel.
resume2.diff (20.6 KB) - added by idk 4 months ago.
Fixed missing !
version-which-limits-rekeys.diff (21.6 KB) - added by idk 4 months ago.
This version of the patch only does one re-key per extended period of idle time. So, if you configured it to re-key after 5 idle minutes, it will do so, but it won't re-key again until some activity occurs on the tunnel. This prevents it from re-keying excessively, even if the rekeyIdleTime is set to the minimum allowable value.
cycledest.png (10.4 KB) - added by Reportage 4 months ago.

Download all attachments as: .zip

Change History (28)

Changed 8 months ago by idk

Attachment: rekey.diff added

Patch which implements the option as i2cp.restartOnIdle

comment:1 Changed 8 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 8 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 8 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 8 months ago by zzz

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

comment:5 Changed 7 months 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 7 months 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 7 months ago by idk (previous) (diff)

comment:7 Changed 7 months 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 7 months ago by idk

Attachment: restart.diff added

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

comment:8 Changed 7 months 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 7 months ago by idk (previous) (diff)

comment:9 Changed 7 months 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 7 months 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 7 months 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 7 months 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) {

Changed 4 months ago by idk

Attachment: resume.diff added

This version uses the existing timer which does reduceOnIdle and closeOnIdle timer to do the whole disconnect, re-key, re-open sequence all at once. However, in order to do it, the _myDestination must not be final. The advantage is that it co-exists just fine with the existing timers, and does not require a second timer. It's also tested and has the expected effect of changing the destination of the client tunnel.

comment:12 Changed 4 months ago by zzz

Style: Some of these were noted way back in comment 1, still not fixed. Please adhere to our style guide.

  • Lots of tabs everywhere, please fix all the spacing and remove the tabs
  • Don't use capital E for a variable: catch (Exception E) - capitalized names are for classes. lower case names are for variables - single upper case chars are for generics.
  • Don't append an exception to an error message - make it the 2nd arg to get the stack trace, e.g. _log.error("Failed reopening " + _session, e):
  • Don't catch Exception - catch the actual exception classes that could be thrown. And if you are just going to catch Exception, you don't need three nested catches in timeReached()
  • Add javadocs with @since to all new methods
  • Lots of WARN logging in rekeySession obviously there for your debugging, but if you have it working, time to remove them

Substance:

  • As _myDestination is no longer final, does it need synchronization of some sort?
  • The default of 10 minutes is great for testing but madness as a default. You're going to generate a new destination (and throw out all your tunnels you worked so hard to build) every 10 minutes? It seems like the default should be several hours.
  • As discussed above, does the time really need to be configurable? If configurable, does it really need to be in the UI?
  • As discussed above, how does this option interact with the related options? What happens if the time is less than the reduce on idle time? More than reduce on idle but less than close on idle? More than close on idle? Are all those cases in your test plan? Do all of them make sense or need to be prevented in the UI and/or code? Also interaction with the new-keys-on-reopen option. See also comment 1 3rd bullet above.
  • more re: interaction: it seems you have some code in timeReached() to resolve some of the conflicts. But none of it is documented in comments, or enforced or explained on the UI side. Need to understand your design intent on what trumps what, then we can evaluate if it works and is presented well to the user. In a lot of the comments above we discussed radio buttons but this is just a checkbox. Need to know the design intent.
  • Tested/works with subsessions? shared clients?
  • Incompatible with persistent client key?
  • Is destroySession() completely synchronous out to the client and in to the router or do we need to sleep after it to be sure? Have you ensured that the raciness of previous approaches is gone, or does it need more review?
  • I think there's definitely locking/race problems with rekeySession() and everything in timeReached()
  • Do changes to the config take effect on a running tunnel? Tested?
  • If a shared client, the new options do affect the other shared clients. But not added to the (new for 0.9.46) shared options logic in GeneralHelper? - see SHARED_OPTIONS
  • In rekeySession(), I see a lot of copypasta from somewhere, but a lot of it doesn't make sense in this context. It feels like you got it from somewhere but didn't think about why. i2pd enc types, offline signatures, transient keys… how could all this happen? Why is it here? Perhaps only if persistent client key? but that's not compatible anyway (see above)
  • why the verifyOpen() call after reconnect() ?

tl;dr I think this patch resolves a lot of my objections I've been repeating for 4 months - that the previous proposals don't work and could never work and were inefficient. It appears that destroySession() is the "signal" through the stack that we needed, although a blunt one. By destroying everything from the bottom up, that solves all the problems of the other layers not "knowing". I think.

But it's still very rough around the edges and needs a lot of polish. You didn't say what your plan is - ready for 47, or this is only for review, or you were now aiming for 48. If for 47, this is where we needed to be about 6 weeks ago, it's getting very late.

comment:13 Changed 4 months ago by idk

  • _myDestination - I don't think so, but I don't know for sure
  • Changed to 3 hours in the latest patch.
  • I prefer it with the configurable timer, but I'm open to leaving it out of the UI.
  • It should be compatible with the other close-on-idle and reduce-on-idle options, in the latest revision I believe it to be. If the time is less than the reduceIdleTime, the tunnel is stopped, re-keyed, re-started, then the tunnels are adjusted back down. If the closeIdleTime is reached, the re-key happens but the tunnel re-open does not. Rekey on re-open still works the same in that it still does it in I2PTunnel. The intent is to be compatible with all three options at once.
  • I will enumerate specific results for shared tunnels and subsessions today.
  • It isn't compatible with persistent client keys. I think I've managed to change the UI so that this will be reflected.
  • rekeySession - This actually ties up with the last style critique re: WARN logging. First: it actually comes from the function directly above rekeySession, readDestination. Second: I figured I didn't actually need to do everything that readDestination did, but I wasn't sure if it may be necessary to have those options. So while I was dogfooding it, I set it up to log exactly what happened during a rekey, configured tunnels with different cryptography and signature types, and watched the log.

I am going to work on getting better answers to the rest of your questions as soon as I can.

Last edited 4 months ago by idk (previous) (diff)

comment:14 Changed 4 months ago by zzz

Thanks. The new diff is a lot easier to read. I'm going to continue researching the (lack of) synchronization issue, on both _myDestination and around rekeySession(). I'll also work on reading the new comments on design intent and seeing if I can convince myself that the code matches the intent.

Changed 4 months ago by idk

Attachment: resume2.diff added

Fixed missing !

Changed 4 months ago by idk

This version of the patch only does one re-key per extended period of idle time. So, if you configured it to re-key after 5 idle minutes, it will do so, but it won't re-key again until some activity occurs on the tunnel. This prevents it from re-keying excessively, even if the rekeyIdleTime is set to the minimum allowable value.

comment:15 Changed 4 months ago by idk

but it won't re-key again until some activity occurs on the tunnel *and a new idle time has begun.

Changed 4 months ago by Reportage

Attachment: cycledest.png added

comment:16 Changed 4 months ago by Reportage

Attaching a prototype UI demonstrating original intent.

comment:17 Changed 4 months ago by zzz

@Reportage re: original intent, does that mean you're unhappy with the UI as implemented in the patches? Please test and review the patches above.

comment:18 Changed 4 months ago by zzz

Comment 13 claims default is now 3 hours but UI still shows 10 minutes

comment:19 Changed 4 months ago by zzz

re: locking

There's an awful lot that could happen in-between when SessionIdleTimer? calls destroySession (line 127) and the call to rekeySession (line 128) returns. None of it is protected by a lock. In particular, an outgoing message could cause the session to reconnect in another thread, I think?

Also, the implementation of the interface-defined methods getMyDestination(), getDecryptionKey(), and getPrivateKey() aren't locked.

Some of the existing code, in particular the close-on-idle, also isn't locked, and we need to look at that closely also.

The choice of what to lock on needs to be carefully considered and tested as we could bump into deadlocks or unexpected behaviors. _stateLock is an obvious candidate but it is complex… see connect()

comment:20 Changed 4 months ago by idk

What @Reportage communicated to me was that having the timer in the UI would be preferred, even if it meant using the same timer for closing and rekeying, and the prototype reflects that. In a future where rekeyOnIdle time is configured with the closeIdleTime option, this is the proposed UI.

I tend to disagree with the idea of using closeIdleTime for rekeyOnIdle, because it forces you to choose between close-on-idle-no-rekey, close-on-idle-rekey-on-reopen and rekey-on-idle, whereas with the approach in my patch the session could rekey after 5 minutes, reduce after 15 minutes, and close after 30 minutes, for example, with no mutual exclusivity between the options or redundant rekeying.

Re: locks I think I, probably naively, assumed that since it would be preceded by a minimum of 5 minutes of idle time, that there would never be such an outgoing message, but having looked at it in practice for a while I think I see your point. It could happen while the actual rekey for instance, is supposed to be occurring, which would be bad. I think I can handle figuring out how to do the locks, though, so I'll just keep working on it.

I'll figure out what's wrong with the default, I think I may have changed it in SessionIdleTimer? but forgot to change it in the UI.

Thanks again for the feedback. :)

comment:21 Changed 3 months ago by Reportage

Re comment:17, I have several issues with the 3 timer approach proposed by idk:

  • Added complexity in the UI
  • Defeats the purpose of permanent availability of tunnel
  • Reducing tunnel count to 1 on idle largely mitigates any resource issues while ensuring that the tunnel is always available without the inherent lag in having to rebuild tunnels


Note: See TracTickets for help on using tickets.