From e3a861187470e1facc6625040128f447ebbcbaec Mon Sep 17 00:00:00 2001
From: "Dylan K. Taylor" <dktapps@pmmp.io>
Date: Mon, 4 Oct 2021 21:39:11 +0100
Subject: [PATCH] Session: Reintroduce delayed sending for disconnect
 notification

this is directed at MCPE to work around the infamous race condition bug
that causes disconnects and transfers to be unreliable.
It reintroduces the old mechanism for server-initiated disconnects that
was used in 0.12:
- First, flush all packets out to clients
- Send disconnect notification
- Wait for ack
- Discard session

On 0.13, it sent the disconnect notification immediately when disconnect
was initiated, which caused the client to sometimes not handle some
packets.
---
 src/server/Session.php | 43 ++++++++++++++++++++++++++++++++----------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/server/Session.php b/src/server/Session.php
index 24d3546..7d0f808 100644
--- a/src/server/Session.php
+++ b/src/server/Session.php
@@ -45,8 +45,9 @@ class Session{
 
 	public const STATE_CONNECTING = 0;
 	public const STATE_CONNECTED = 1;
-	public const STATE_DISCONNECTING = 2;
-	public const STATE_DISCONNECTED = 3;
+	public const STATE_DISCONNECT_PENDING = 2;
+	public const STATE_DISCONNECT_NOTIFIED = 3;
+	public const STATE_DISCONNECTED = 4;
 
 	public const MIN_MTU_SIZE = 400;
 
@@ -149,7 +150,10 @@ public function isTemporal() : bool{
 	}
 
 	public function isConnected() : bool{
-		return $this->state !== self::STATE_DISCONNECTING and $this->state !== self::STATE_DISCONNECTED;
+		return
+			$this->state !== self::STATE_DISCONNECT_PENDING and
+			$this->state !== self::STATE_DISCONNECT_NOTIFIED and
+			$this->state !== self::STATE_DISCONNECTED;
 	}
 
 	public function update(float $time) : void{
@@ -159,12 +163,18 @@ public function update(float $time) : void{
 			return;
 		}
 
-		if($this->state === self::STATE_DISCONNECTING){
+		if($this->state === self::STATE_DISCONNECT_PENDING || $this->state === self::STATE_DISCONNECT_NOTIFIED){
 			//by this point we already told the event listener that the session is closing, so we don't need to do it again
 			if(!$this->sendLayer->needsUpdate() and !$this->recvLayer->needsUpdate()){
-				$this->state = self::STATE_DISCONNECTED;
-				$this->logger->debug("Client cleanly disconnected, marking session for destruction");
-				return;
+				if($this->state === self::STATE_DISCONNECT_PENDING){
+					$this->queueConnectedPacket(new DisconnectionNotification(), PacketReliability::RELIABLE_ORDERED, 0, true);
+					$this->state = self::STATE_DISCONNECT_NOTIFIED;
+					$this->logger->debug("All pending traffic flushed, sent disconnect notification");
+				}else{
+					$this->state = self::STATE_DISCONNECTED;
+					$this->logger->debug("Client cleanly disconnected, marking session for destruction");
+					return;
+				}
 			}elseif($this->disconnectionTime + 10 < $time){
 				$this->state = self::STATE_DISCONNECTED;
 				$this->logger->debug("Timeout during graceful disconnect, forcibly closing session");
@@ -238,7 +248,7 @@ private function handleEncapsulatedPacketRoute(EncapsulatedPacket $packet) : voi
 					}
 				}
 			}elseif($id === DisconnectionNotification::$ID){
-				$this->initiateDisconnect("client disconnect");
+				$this->onClientDisconnect();
 			}elseif($id === ConnectedPing::$ID){
 				$dataPacket = new ConnectedPing();
 				$dataPacket->decode(new PacketSerializer($packet->buffer));
@@ -290,9 +300,8 @@ public function handlePacket(Packet $packet) : void{
 	 */
 	public function initiateDisconnect(string $reason) : void{
 		if($this->isConnected()){
-			$this->state = self::STATE_DISCONNECTING;
+			$this->state = self::STATE_DISCONNECT_PENDING;
 			$this->disconnectionTime = microtime(true);
-			$this->queueConnectedPacket(new DisconnectionNotification(), PacketReliability::RELIABLE_ORDERED, 0, true);
 			$this->server->getEventListener()->onClientDisconnect($this->internalId, $reason);
 			$this->logger->debug("Requesting graceful disconnect because \"$reason\"");
 		}
@@ -307,6 +316,20 @@ public function forciblyDisconnect(string $reason) : void{
 		$this->logger->debug("Forcibly disconnecting session due to \"$reason\"");
 	}
 
+	private function onClientDisconnect() : void{
+		//the client will expect an ACK for this; make sure it gets sent, because after forcible termination
+		//there won't be any session ticks to update it
+		$this->recvLayer->update();
+
+		if($this->isConnected()){
+			//the client might have disconnected after the server sent a disconnect notification, but before the client
+			//received it - in this case, we don't want to notify the event handler twice
+			$this->server->getEventListener()->onClientDisconnect($this->internalId, "client disconnect");
+		}
+		$this->state = self::STATE_DISCONNECTED;
+		$this->logger->debug("Terminating session due to client disconnect");
+	}
+
 	/**
 	 * Returns whether the session is ready to be destroyed (either properly cleaned up or forcibly terminated)
 	 */