Redis followup from #272

src/RedisHandler.php

@@ -3,16 +3,50 @@
 
 use Redis;
 use RuntimeException;
+use Throwable;
 
 class RedisHandler extends Handler {
+	private const EMPTY_PHP_ARRAY = "a:0:{}";
 	private const DEFAULT_PORT = 6379;
 	private const DEFAULT_PREFIX_SEPARATOR = ":";
 	private ?Redis $client = null;
+	/** @var array{
+	 *   host:string,
+	 *   port:int,
+	 *   timeout:float,
+	 *   readTimeout:float,
+	 *   persistentId:?string,
+	 *   prefix:string,
+	 *   ttl:int,
+	 *   database:int,
+	 *   auth:array{string,string}|string|null,
+	 *   context:array{stream:array{verify_peer:bool,verify_peer_name:bool}}|null
+	 * }|null
+	 */
+	private ?array $config = null;
 	private string $prefix = "";
 	private int $ttl = 0;
 
 	public function open(string $savePath, string $name):bool {
 		$config = $this->parseSavePath($savePath, $name);
+		$this->config = $config;
+		return $this->connect($config);
+	}
+
+	/** @param array{
+	 *   host:string,
+	 *   port:int,
+	 *   timeout:float,
+	 *   readTimeout:float,
+	 *   persistentId:?string,
+	 *   prefix:string,
+	 *   ttl:int,
+	 *   database:int,
+	 *   auth:array{string,string}|string|null,
+	 *   context:array{stream:array{verify_peer:bool,verify_peer_name:bool}}|null
+	 * } $config
+	 */
+	private function connect(array $config):bool {
 		$client = $this->createClient();
 
 		$connected = $client->connect(
@@ -48,27 +82,46 @@ public function close():bool {
 			return true;
 		}
 
-		return $this->client->close();
+		try {
+			return $this->client->close();
+		}
+		catch(Throwable) {
+			return true;
+		}
+		finally {
+			$this->client = null;
+		}
 	}
 
 	public function read(string $sessionId):string {
-		$value = $this->requireClient()->get($this->getKey($sessionId));
+		$value = $this->retryOnce(
+			fn() => $this->requireClient()->get($this->getKey($sessionId))
+		);
 		return is_string($value) ? $value : "";
 	}
 
 	public function write(string $sessionId, string $sessionData):bool {
-		$client = $this->requireClient();
+		if($sessionData === self::EMPTY_PHP_ARRAY) {
+			return true;
+		}
+
 		$key = $this->getKey($sessionId);
 
 		if($this->ttl > 0) {
-			return $client->setEx($key, $this->ttl, $sessionData);
+			return $this->retryOnce(
+				fn() => $this->requireClient()->setEx($key, $this->ttl, $sessionData)
+			);
 		}
 
-		return $client->set($key, $sessionData);
+		return $this->retryOnce(
+			fn() => $this->requireClient()->set($key, $sessionData)
+		);
 	}
 
 	public function destroy(string $id = ""):bool {
-		return $this->requireClient()->del($this->getKey($id)) >= 0;
+		return $this->retryOnce(
+			fn() => $this->requireClient()->del($this->getKey($id))
+		) >= 0;
 	}
 
 	// phpcs:disable Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClass
@@ -219,4 +272,28 @@ private function requireClient():Redis {
 
 		return $this->client;
 	}
+
+	private function reconnect():void {
+		if(is_null($this->config)) {
+			throw new RuntimeException("RedisHandler::open() must be called before reconnect.");
+		}
+
+		$this->close();
+		$this->connect($this->config);
+	}
+
+	/**
+	 * @template T
+	 * @param callable():T $callback
+	 * @return T
+	 */
+	private function retryOnce(callable $callback):mixed {
+		try {
+			return $callback();
+		}
+		catch(Throwable) {
+			$this->reconnect();
+			return $callback();
+		}
+	}
 }

test/phpunit/RedisHandlerTest.php

@@ -5,6 +5,11 @@
 use PHPUnit\Framework\TestCase;
 use Redis;
 
+if(!class_exists(Redis::class)) {
+	class TestRedisClientBase {}
+	class_alias(TestRedisClientBase::class, "Redis");
+}
+
 class RedisHandlerTest extends TestCase {
 	public function testOpenParsesStandardDsn():void {
 		$client = new TestRedisClient();
@@ -91,9 +96,44 @@ protected function createClient():Redis {
 		self::assertTrue($sut->close());
 		self::assertTrue($client->closed);
 	}
+
+	public function testEmptyNativePhpSessionWriteDoesNotOverwriteStoredSession():void {
+		$client = new TestRedisClient();
+		$sut = new class($client) extends RedisHandler {
+			public function __construct(private readonly TestRedisClient $client) {}
+
+			protected function createClient():Redis {
+				/** @phpstan-ignore-next-line */
+				return $this->client;
+			}
+		};
+		$sut->open("redis://cache.internal", "GT");
+		$sut->write("abc123", "stored-session");
+		$sut->write("abc123", "a:0:{}");
+
+		self::assertSame("stored-session", $sut->read("abc123"));
+	}
+
+	public function testCommandRetriesAfterDisconnectedClient():void {
+		$client = new TestRedisClient();
+		$sut = new class($client) extends RedisHandler {
+			public function __construct(private readonly TestRedisClient $client) {}
+
+			protected function createClient():Redis {
+				/** @phpstan-ignore-next-line */
+				return $this->client;
+			}
+		};
+		$sut->open("redis://cache.internal", "GT");
+		$client->failNextSetEx = true;
+
+		self::assertTrue($sut->write("abc123", "payload"));
+		self::assertSame("payload", $sut->read("abc123"));
+		self::assertSame(2, $client->connectCount);
+	}
 }
 
-class TestRedisClient {
+class TestRedisClient extends Redis {
 	/** @var array<string,mixed> */
 	public array $connectParameters = [];
 	/** @var array<int,array{key:string,ttl:int,value:string}> */
@@ -107,27 +147,30 @@ class TestRedisClient {
 	/** @var array<string,string> */
 	public array $data = [];
 	public int $deleted = 0;
+	public int $connectCount = 0;
+	public bool $failNextSetEx = false;
 	public bool $closed = false;
 
 	/**
 	 * @param array{auth?:array{0:string|false|null,1?:string},stream?:array<string,mixed>}|null $context
 	 */
 	public function connect(
 		string $host,
-		int $port,
+		int $port = 6379,
 		float $timeout = 0,
-		?string $persistentId = null,
-		int $retryInterval = 0,
-		float $readTimeout = 0,
+		?string $persistent_id = null,
+		int $retry_interval = 0,
+		float $read_timeout = 0,
 		?array $context = null,
 	):bool {
+		$this->connectCount++;
 		$this->connectParameters = [
 			"host" => $host,
 			"port" => $port,
 			"timeout" => $timeout,
-			"persistentId" => $persistentId,
-			"retryInterval" => $retryInterval,
-			"readTimeout" => $readTimeout,
+			"persistentId" => $persistent_id,
+			"retryInterval" => $retry_interval,
+			"readTimeout" => $read_timeout,
 			"context" => $context,
 		];
 		return true;
@@ -136,12 +179,12 @@ public function connect(
 	/**
 	 * @param array{string,string}|string $credentials
 	 */
-	public function auth(array|string $credentials):bool {
+	public function auth(mixed $credentials):Redis|bool {
 		$this->authCalls []= $credentials;
 		return true;
 	}
 
-	public function select(int $database):bool {
+	public function select(int $database):Redis|bool {
 		$this->selectCalls []= $database;
 		return true;
 	}
@@ -150,23 +193,32 @@ public function get(string $key):string|false {
 		return $this->data[$key] ?? false;
 	}
 
-	public function set(string $key, string $value):bool {
+	public function set(string $key, mixed $value, mixed $options = null):Redis|string|bool {
 		$this->setCalls []= $key;
-		$this->data[$key] = $value;
+		$this->data[$key] = (string)$value;
 		return true;
 	}
 
-	public function setEx(string $key, int $ttl, string $value):bool {
+	public function setEx(string $key, int $ttl, mixed $value) {
+		if($this->failNextSetEx) {
+			$this->failNextSetEx = false;
+			throw new \RedisException("Redis server went away");
+		}
+
 		$this->setExCalls []= [
 			"key" => $key,
 			"ttl" => $ttl,
-			"value" => $value,
+			"value" => (string)$value,
 		];
-		$this->data[$key] = $value;
+		$this->data[$key] = (string)$value;
 		return true;
 	}
 
-	public function del(string $key):int {
+	public function del(array|string $key, string ...$otherKeys):Redis|int|false {
+		if(is_array($key)) {
+			$key = reset($key);
+		}
+
 		unset($this->data[$key]);
 		return ++$this->deleted;
 	}
@@ -176,7 +228,3 @@ public function close():bool {
 		return true;
 	}
 }
-
-if(!class_exists(Redis::class)) {
-	class_alias(TestRedisClient::class, Redis::class);
-}