From a39496a125170163a93c557547156f7073c0e532 Mon Sep 17 00:00:00 2001
From: Simon Rettberg
Date: Mon, 20 Jul 2020 17:01:17 +0200
Subject: Refine timeout handling of dead/removed clients

A client that was just claimed gets a one minute grace period before the
"no active tunnel detection" can kick in. This prevents a race where the
client gets claimed and then a json update is performed before the
tunnel connection is established.

Also the removal of clients that are missing from the remote meta data
is hopefully more robust now, not removing a client as long as there is
an active user and an open tunnel connection.
---
 .../de/bwlehrpool/bwlp_guac/AvailableClient.java   | 49 ++++++++++++++++------
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/src/main/java/de/bwlehrpool/bwlp_guac/AvailableClient.java b/src/main/java/de/bwlehrpool/bwlp_guac/AvailableClient.java
index 5371547..aa7506d 100644
--- a/src/main/java/de/bwlehrpool/bwlp_guac/AvailableClient.java
+++ b/src/main/java/de/bwlehrpool/bwlp_guac/AvailableClient.java
@@ -36,7 +36,15 @@ public class AvailableClient implements Cloneable {
 
 	private WrappedConnection connection;
 
+	/**
+	 * TS when to delete this entry, or 0 = never
+	 */
 	private long deadline;
+	
+	/**
+	 * TA when to release this client if it's claimed, but state is not occupied
+	 */
+	private long idleClaimTimeout;
 
 	private long lastConnectionCheck;
 
@@ -60,9 +68,9 @@ public class AvailableClient implements Cloneable {
 	 * can't possibly be in use by the old user anymore.
 	 */
 	public synchronized void update(JsonClient source) {
-		if (this.inUseBy != null && source.state != State.OCCUPIED && !TunnelListener.hasTunnel(this.inUseBy)) {
-			LOGGER.info("Free client blocked by a disconnected user detected.");
-			LOGGER.info("Client " + this + " is available again");
+		if (this.inUseBy != null && source.state != State.OCCUPIED && !TunnelListener.hasTunnel(this.inUseBy)
+				&& System.currentTimeMillis() > this.idleClaimTimeout) {
+			LOGGER.info("Client " + this + " is available again (free client blocked by a user)");
 			this.inUseBy = null;
 			if (this.connection != null) {
 				this.connection.invalidate();
@@ -70,19 +78,20 @@ public class AvailableClient implements Cloneable {
 		}
 
 		if (this.password == null || !this.password.equals(source.password)) {
-			if (source.state != State.OCCUPIED) {
-				if (this.inUseBy != null) {
-					LOGGER.info("Client " + this + " is available again");
-					this.inUseBy = null;
-					if (this.connection != null) this.connection.invalidate();
+			if (source.state != State.OCCUPIED && this.inUseBy != null) {
+				LOGGER.info("Client " + this + " is available again");
+				this.inUseBy = null;
+				if (this.connection != null) {
+					this.connection.invalidate();
 				}
 			}
 			this.lastConnectionCheck = 0;
 			this.password = source.password;
 		}
 
-		if (this.inUseBy == null || source.state != State.IDLE)
+		if (this.inUseBy == null || source.state != State.IDLE) {
 			this.state = source.state;
+		}
 
 		this.deadline = 0;
 	}
@@ -95,6 +104,7 @@ public class AvailableClient implements Cloneable {
 			return false;
 		this.inUseBy = user;
 		this.state = State.OCCUPIED;
+		this.idleClaimTimeout = System.currentTimeMillis() + 60000; // Give connect and login some time
 		return true;
 	}
 
@@ -113,7 +123,9 @@ public class AvailableClient implements Cloneable {
 
 	public synchronized void releaseConnection(String expectedOwner) {
 		if (isInUseBy(expectedOwner)) {
-			if (this.connection != null) this.connection.invalidate();
+			if (this.connection != null) {
+				this.connection.invalidate();
+			}
 			LOGGER.info("Prematurely releasing client " + this);
 			this.inUseBy = null;
 		} else {
@@ -123,14 +135,25 @@ public class AvailableClient implements Cloneable {
 
 	/**
 	 * If this connection is not returned by the sat server anymore, we keep it
-	 * around for another 5 minutes just in case.
+	 * around as long as there is someone using it, with some safety buffer.
 	 */
 	public boolean isTimeout(long NOW) {
 		if (deadline == 0) {
-			deadline = NOW + 300000;
+			deadline = NOW + 120000; // Was recently updated from remote, set 2min timeout
+			return false;
+		}
+		if (NOW < deadline) // Not timed out yet, no need to recheck
 			return false;
+		long remaining = deadline - NOW;
+		if (remaining < 120000) {
+			synchronized (this) {
+				if (this.inUseBy != null && TunnelListener.hasTunnel(this.inUseBy)) {
+					deadline = NOW + 600000; // Another 10 minutes since the tunnel is active
+					LOGGER.info("Extending deadline of vanished client, because it's in use: " + this);
+				}
+			}
 		}
-		return deadline < NOW;
+		return NOW > deadline;
 	}
 
 	@Override
-- 
cgit v1.2.3-55-g7522