From 222697aee875225ce7290c8d0049d36cb01ef246 Mon Sep 17 00:00:00 2001 From: Jakob Gillich Date: Tue, 26 May 2020 22:21:49 +0200 Subject: [PATCH] Refactor MultiPacketCollector#Bundle (#467) Bundle was never called in any place other than BundleOption. This refactors BundleOption into Bundle and removes BundleOption. Besides being more efficient, it no longer has the possibility of swallowing unrelated exceptions. --- .../packet/control/MultiPacketCollector.scala | 30 ++----- .../control/MultiPacketCollectorTest.scala | 83 +++++++++++-------- .../src/main/scala/WorldSessionActor.scala | 6 +- 3 files changed, 59 insertions(+), 60 deletions(-) diff --git a/common/src/main/scala/net/psforever/packet/control/MultiPacketCollector.scala b/common/src/main/scala/net/psforever/packet/control/MultiPacketCollector.scala index 16afb22c..32c18001 100644 --- a/common/src/main/scala/net/psforever/packet/control/MultiPacketCollector.scala +++ b/common/src/main/scala/net/psforever/packet/control/MultiPacketCollector.scala @@ -74,32 +74,16 @@ class MultiPacketCollector() { /** * Retrieve the internal collection of packets. * Reset the internal list of packets by clearing it. - * @return a loaded `MultiPacketBundle` object - */ - def Bundle : MultiPacketBundle = { - try { - val out = MultiPacketBundle(bundle) - bundle = List.empty - out - } - catch { - case _ : Exception => //catch and rethrow the exception - throw new RuntimeException("no packets") - } - } - - /** - * A safer `Bundle` that consumes any` Exceptions` that might be thrown in the process of producing output. - * @see `Bundle` * @return a loaded `MultiPacketBundle` object, or `None` */ - def BundleOption : Option[MultiPacketBundle] = { - try { - Some(Bundle) - } - catch { - case _ : Exception => + def Bundle : Option[MultiPacketBundle] = { + bundle match { + case Nil => None + case list => + val out = MultiPacketBundle(list) + bundle = List.empty + Some(out) } } } diff --git a/common/src/test/scala/control/MultiPacketCollectorTest.scala b/common/src/test/scala/control/MultiPacketCollectorTest.scala index bf850a9d..71dbae0e 100644 --- a/common/src/test/scala/control/MultiPacketCollectorTest.scala +++ b/common/src/test/scala/control/MultiPacketCollectorTest.scala @@ -78,7 +78,7 @@ class MultiPacketCollectorTest extends Specification { "can retrieve a bundle packets" in { val obj = MultiPacketCollector(List(packet1, packet2)) obj.Bundle match { - case MultiPacketBundle(list) => + case Some(MultiPacketBundle(list)) => list.size mustEqual 2 list.head mustEqual packet1 list(1) mustEqual packet2 @@ -87,21 +87,16 @@ class MultiPacketCollectorTest extends Specification { } } - "can not retrieve a bundle of non-existent packets" in { - val obj = new MultiPacketCollector() - obj.Bundle must throwA[RuntimeException] - } - - "can safely retrieve a bundle of potential packets" in { + "can retrieve a bundle of potential packets" in { val obj1 = new MultiPacketCollector() - obj1.BundleOption match { + obj1.Bundle match { case Some(_) => ko case _ => ; } val obj2 = MultiPacketCollector(List(packet1, packet2)) - obj2.BundleOption match { + obj2.Bundle match { case None => ko case Some(MultiPacketBundle(list)) => @@ -114,15 +109,27 @@ class MultiPacketCollectorTest extends Specification { "clear packets after being asked to bundle" in { val list = List(packet1, packet2) val obj = MultiPacketCollector(list) - obj.Bundle mustEqual MultiPacketBundle(list) - obj.Bundle must throwA[RuntimeException] + + obj.Bundle match { + case Some(MultiPacketBundle(bundle)) => + bundle mustEqual list + case _ => + ko + } + + obj.Bundle match { + case Some(MultiPacketBundle(_)) => + ko + case _ => + ok + } } "add a packet" in { val obj = new MultiPacketCollector() obj.Add(packet1) obj.Bundle match { - case MultiPacketBundle(list) => + case Some(MultiPacketBundle(list)) => list.size mustEqual 1 list.head mustEqual packet1 case _ => @@ -134,7 +141,7 @@ class MultiPacketCollectorTest extends Specification { val obj = new MultiPacketCollector() obj.Add(List(packet1, packet2)) obj.Bundle match { - case MultiPacketBundle(list) => + case Some(MultiPacketBundle(list)) => list.size mustEqual 2 list.head mustEqual packet1 list(1) mustEqual packet2 @@ -147,38 +154,46 @@ class MultiPacketCollectorTest extends Specification { val obj1 = new MultiPacketCollector() obj1.Add(List(packet1, packet2)) - val bundle1 = obj1.Bundle - - val obj2 = MultiPacketCollector(bundle1) - obj2.Add(packet3) - obj2.Bundle match { - case MultiPacketBundle(list) => - list.size mustEqual 3 - list.head mustEqual packet1 - list(1) mustEqual packet2 - list(2) mustEqual packet3 + obj1.Bundle match { + case Some(MultiPacketBundle(bundle1)) => + val obj2 = MultiPacketCollector(bundle1) + obj2.Add(packet3) + obj2.Bundle match { + case Some(MultiPacketBundle(list)) => + list.size mustEqual 3 + list.head mustEqual packet1 + list(1) mustEqual packet2 + list(2) mustEqual packet3 + case _ => + ko + } case _ => ko } + } "concatenate bundles (2)" in { val obj1 = new MultiPacketCollector() obj1.Add(List(packet1, packet2)) - val bundle1 = obj1.Bundle - - val obj2 = new MultiPacketCollector() - obj2.Add(packet3) - obj2.Add(bundle1) - obj2.Bundle match { - case MultiPacketBundle(list) => - list.size mustEqual 3 - list.head mustEqual packet3 - list(1) mustEqual packet1 - list(2) mustEqual packet2 + obj1.Bundle match { + case Some(MultiPacketBundle(bundle1)) => + val obj2 = new MultiPacketCollector() + obj2.Add(packet3) + obj2.Add(bundle1) + obj2.Bundle match { + case Some(MultiPacketBundle(list))=> + list.size mustEqual 3 + list.head mustEqual packet3 + list(1) mustEqual packet1 + list(2) mustEqual packet2 + case _ => + ko + } case _ => ko } + } } } diff --git a/pslogin/src/main/scala/WorldSessionActor.scala b/pslogin/src/main/scala/WorldSessionActor.scala index 27a4f4e4..b8e3cef1 100644 --- a/pslogin/src/main/scala/WorldSessionActor.scala +++ b/pslogin/src/main/scala/WorldSessionActor.scala @@ -3637,7 +3637,7 @@ class WorldSessionActor extends Actor /** * Instruct the client to treat this player as the avatar. - * Initialize all client-specific data that is dependent on some player beign decalred the "avatar". + * Initialize all client-specific data that is dependent on some player being declared the "avatar". * @param tplayer the target player */ def HandleSetCurrentAvatar(tplayer : Player) : Unit = { @@ -11445,7 +11445,7 @@ class WorldSessionActor extends Actor def StopBundlingPackets() : Unit = { log.trace("WORLD SEND: PACKET BUNDLING SUSPENDED") packetBundlingFunc = NoBundlingAction - packetBundlingCollector.BundleOption match { + packetBundlingCollector.Bundle match { case Some(bundle) => sendResponse(bundle) case None => ; @@ -11501,7 +11501,7 @@ class WorldSessionActor extends Actor */ def sendResponse(cont : KeepAliveMessage) : Unit = { sendResponse(PacketCoding.CreateGamePacket(0, cont)) - packetBundlingCollector.BundleOption match { + packetBundlingCollector.Bundle match { case Some(bundle) => log.trace("WORLD SEND: INTERMITTENT PACKET BUNDLE") sendResponse(bundle)