From 562f047f03651e9ed817dda526e180262608ce60 Mon Sep 17 00:00:00 2001
From: wixoa <wixoag@gmail.com>
Date: Tue, 26 Dec 2023 15:39:08 -0500
Subject: [PATCH] Make ServerAppearanceSystem thread-safe (#1568)

* Use locks on ServerAppearanceSystem

* Use a file-scoped namespace
---
 .../Rendering/ServerAppearanceSystem.cs       | 59 ++++++++++++-------
 1 file changed, 37 insertions(+), 22 deletions(-)

diff --git a/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs b/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs
index abd927ba06..81b96969dd 100644
--- a/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs
+++ b/OpenDreamRuntime/Rendering/ServerAppearanceSystem.cs
@@ -5,31 +5,40 @@
 using System.Diagnostics.CodeAnalysis;
 using Robust.Shared.Player;
 
-namespace OpenDreamRuntime.Rendering {
-    public sealed class ServerAppearanceSystem : SharedAppearanceSystem {
-        private readonly Dictionary<IconAppearance, int> _appearanceToId = new();
-        private readonly Dictionary<int, IconAppearance> _idToAppearance = new();
-        private int _appearanceIdCounter;
+namespace OpenDreamRuntime.Rendering;
 
-        [Dependency] private readonly IPlayerManager _playerManager = default!;
+public sealed class ServerAppearanceSystem : SharedAppearanceSystem {
+    private readonly Dictionary<IconAppearance, int> _appearanceToId = new();
+    private readonly Dictionary<int, IconAppearance> _idToAppearance = new();
+    private int _appearanceIdCounter;
 
-        public override void Initialize() {
-            _playerManager.PlayerStatusChanged += OnPlayerStatusChanged;
-        }
+    /// <summary>
+    /// This system is used by the PVS thread, we need to be thread-safe
+    /// </summary>
+    private readonly object _lock = new();
+
+    [Dependency] private readonly IPlayerManager _playerManager = default!;
+
+    public override void Initialize() {
+        _playerManager.PlayerStatusChanged += OnPlayerStatusChanged;
+    }
 
-        public override void Shutdown() {
+    public override void Shutdown() {
+        lock (_lock) {
             _appearanceToId.Clear();
             _idToAppearance.Clear();
             _appearanceIdCounter = 0;
         }
+    }
 
-        private void OnPlayerStatusChanged(object? sender, SessionStatusEventArgs e) {
-            if (e.NewStatus == SessionStatus.InGame) {
-                RaiseNetworkEvent(new AllAppearancesEvent(_idToAppearance), e.Session.ConnectedClient);
-            }
+    private void OnPlayerStatusChanged(object? sender, SessionStatusEventArgs e) {
+        if (e.NewStatus == SessionStatus.InGame) {
+            RaiseNetworkEvent(new AllAppearancesEvent(_idToAppearance), e.Session.ConnectedClient);
         }
+    }
 
-        public int AddAppearance(IconAppearance appearance) {
+    public int AddAppearance(IconAppearance appearance) {
+        lock (_lock) {
             if (!_appearanceToId.TryGetValue(appearance, out int appearanceId)) {
                 appearanceId = _appearanceIdCounter++;
                 _appearanceToId.Add(appearance, appearanceId);
@@ -39,23 +48,29 @@ public int AddAppearance(IconAppearance appearance) {
 
             return appearanceId;
         }
+    }
 
-        public IconAppearance MustGetAppearance(int appearanceId) {
+    public IconAppearance MustGetAppearance(int appearanceId) {
+        lock (_lock) {
             return _idToAppearance[appearanceId];
         }
+    }
 
-        public bool TryGetAppearance(int appearanceId, [NotNullWhen(true)] out IconAppearance? appearance) {
+    public bool TryGetAppearance(int appearanceId, [NotNullWhen(true)] out IconAppearance? appearance) {
+        lock (_lock) {
             return _idToAppearance.TryGetValue(appearanceId, out appearance);
         }
+    }
 
-        public bool TryGetAppearanceId(IconAppearance appearance, out int appearanceId) {
+    public bool TryGetAppearanceId(IconAppearance appearance, out int appearanceId) {
+        lock (_lock) {
             return _appearanceToId.TryGetValue(appearance, out appearanceId);
         }
+    }
 
-        public void Animate(NetEntity entity, IconAppearance targetAppearance, TimeSpan duration) {
-            int appearanceId = AddAppearance(targetAppearance);
+    public void Animate(NetEntity entity, IconAppearance targetAppearance, TimeSpan duration) {
+        int appearanceId = AddAppearance(targetAppearance);
 
-            RaiseNetworkEvent(new AnimationEvent(entity, appearanceId, duration));
-        }
+        RaiseNetworkEvent(new AnimationEvent(entity, appearanceId, duration));
     }
 }