ADOPTED: Datum softdels

Content.Tests/DMTests.cs

@@ -95,6 +95,9 @@ public void TestFiles(string sourceFile, DMTestFlags testFlags) {
                 Assert.That(returned?.IsTruthy(), Is.True, "Test was expected to return TRUE");
             }
 
+            // Due to how softdels work we need to make sure we clean up /now/.
+            GC.Collect();
+            _dreamMan.Update();
             Cleanup(compiledFile);
             TestContext.WriteLine($"--- PASS {sourceFile}");
         } finally {

DMCompiler/DMStandard/Types/Datum.dm

@@ -7,7 +7,8 @@
 	var/tag
 
 	proc/New()
-
+	//SAFETY: If you redefine this to anything except empty, please revisit how DreamObject handles Del() or it will
+	//        attempt to run DM on a GC thread, potentially causing problems.
 	proc/Del()
 
 	proc/Topic(href, href_list)

OpenDreamRuntime/DreamManager.cs

@@ -1,6 +1,8 @@
-using System.IO;
+using System.Collections.Concurrent;
+using System.IO;
 using System.Linq;
 using System.Text.Json;
+using System.Threading;
 using DMCompiler.Bytecode;
 using DMCompiler.Json;
 using OpenDreamRuntime.Objects;
@@ -9,6 +11,7 @@
 using OpenDreamRuntime.Procs.Native;
 using OpenDreamRuntime.Rendering;
 using OpenDreamRuntime.Resources;
+using OpenDreamRuntime.Util;
 using OpenDreamShared;
 using OpenDreamShared.Dream;
 using Robust.Server;
@@ -44,10 +47,11 @@
         // Global state that may not really (really really) belong here
         public DreamValue[] Globals { get; set; } = Array.Empty<DreamValue>();
         public List<string> GlobalNames { get; private set; } = new();
-        public Dictionary<DreamObject, int> ReferenceIDs { get; } = new();
-        public Dictionary<int, DreamObject> ReferenceIDsToDreamObject { get; } = new();
+        public ConcurrentDictionary<int, WeakDreamRef> ReferenceIDsToDreamObject { get; } = new();
         public HashSet<DreamObject> Clients { get; set; } = new();
-        public HashSet<DreamObject> Datums { get; set; } = new();
+        // I solemnly swear this benefits from being a linked list (constant remove times without relying on object hash) --kaylie
+        public LinkedList<WeakDreamRef> Datums { get; set; } = new();
+        public ConcurrentBag<DreamObject> DelQueue = new();
         public Random Random { get; set; } = new();
         public Dictionary<string, List<DreamObject>> Tags { get; set; } = new();
         public DreamProc ImageConstructor, ImageFactoryProc;
@@ -103,6 +107,13 @@
             _dreamMapManager.UpdateTiles();
             DreamObjectSavefile.FlushAllUpdates();
             WorldInstance.SetVariableValue("cpu", WorldInstance.GetVariable("tick_usage"));
+            ProcessDelQueue();
+        }
+
+        public void ProcessDelQueue() {
+            while (DelQueue.TryTake(out var obj)) {
+                obj.Delete();
+            }
         }
 
         public bool LoadJson(string? jsonPath) {
@@ -205,11 +216,14 @@
                             break;
                         }
                     }
-
-                    if (!ReferenceIDs.TryGetValue(refObject, out idx)) {
-                        idx = _dreamObjectRefIdCounter++;
-                        ReferenceIDs.Add(refObject, idx);
-                        ReferenceIDsToDreamObject.Add(idx, refObject);
+                    if (refObject.RefID is not {} id) {
+                        idx = Interlocked.Increment(ref _dreamObjectRefIdCounter);
+                        refObject.RefID = idx;
+
+                        // SAFETY: Infallible! idx is always unique and add can only fail if this is not the case.
+                        ReferenceIDsToDreamObject.TryAdd(idx, new WeakDreamRef(refObject));
+                    } else {
+                        idx = id;
                     }
                 }
             } else if (value.TryGetValueAsString(out var refStr)) {
@@ -241,6 +255,28 @@
             return $"[0x{((int) refType+idx):x}]";
         }
 
+        /// <summary>
+        /// Iterates the list of datums
+        /// </summary>
+        /// <returns>Datum enumerater</returns>
+        /// <remarks>As it's a convenient time, this will collect any dead datum refs as it finds them.</remarks>
+        public IEnumerable<DreamObject> IterateDatums() {
+            // This isn't a common operation so we'll use this time to also do some pruning.
+            var node = Datums.First;
+
+            while (node is not null) {
+                var next = node.Next;
+                var val = node.Value.Target;
+                if (val is null)
+                    Datums.Remove(node);
+                else
+                    yield return val;
+                node = next;
+            }
+
+            yield break;
+        }
+
         public DreamValue LocateRef(string refString) {
             bool canBePointer = false;
 
@@ -269,7 +305,7 @@
                     case RefType.DreamObjectMob:
                     case RefType.DreamObjectTurf:
                     case RefType.DreamObject:
-                        if (ReferenceIDsToDreamObject.TryGetValue(refId, out var dreamObject))
+                        if (ReferenceIDsToDreamObject.TryGetValue(refId, out var weakRef) && weakRef.Target is {} dreamObject)
                             return new(dreamObject);
 
                         return DreamValue.Null;

OpenDreamRuntime/Objects/DreamObject.cs

@@ -1,11 +1,13 @@
-using System.Diagnostics.CodeAnalysis;
+using System.Collections.Concurrent;
+using System.Diagnostics.CodeAnalysis;
 using OpenDreamRuntime.Procs;
 using System.Globalization;
 using System.Runtime.CompilerServices;
 using DMCompiler.Bytecode;
 using OpenDreamRuntime.Objects.Types;
 using OpenDreamRuntime.Rendering;
 using OpenDreamRuntime.Resources;
+using OpenDreamRuntime.Util;
 using Robust.Server.GameObjects;
 using Robust.Server.GameStates;
 using Robust.Server.Player;
@@ -21,6 +23,9 @@
         [Access(typeof(DreamObject))]
         public bool Deleted;
 
+        [Access(typeof(DreamManager), typeof(DreamObject))]
+        public int? RefID;
+
         public virtual bool ShouldCallNew => true;
 
         // Shortcuts to IoC dependencies & entity systems
@@ -81,22 +86,20 @@
 
             // Atoms are in world.contents
             if (this is not DreamObjectAtom && IsSubtypeOf(ObjectTree.Datum)) {
-                ObjectDefinition.DreamManager.Datums.Add(this);
+                ObjectDefinition.DreamManager.Datums.AddLast(new WeakDreamRef(this));
             }
         }
 
         public virtual void Initialize(DreamProcArguments args) {
             // For subtypes to implement
         }
 
-        protected virtual void HandleDeletion() {
+        protected virtual void HandleDeletion(bool possiblyThreaded) {
             // Atoms are in world.contents
-            if (this is not DreamObjectAtom && IsSubtypeOf(ObjectTree.Datum)) {
-                DreamManager.Datums.Remove(this);
-            }
+            // Datum removal used to live here, but datums are now tracked weakly.
 
-            if (DreamManager.ReferenceIDs.Remove(this, out var refId))
-                DreamManager.ReferenceIDsToDreamObject.Remove(refId);
+            if (RefID is not null)
+                DreamManager.ReferenceIDsToDreamObject.Remove(RefID.Value, out _);
 
             Tag = null;
             Deleted = true;
@@ -106,14 +109,41 @@
             ObjectDefinition = null!;
         }
 
-        public void Delete() {
+
+        /// <summary>
+        ///     Enters the current dream object into a global del queue that is guaranteed to run on the DM thread.
+        ///     Use if your deletion handler must be on the DM thread.
+        /// </summary>
+        protected void EnterIntoDelQueue() {
+            DreamManager.DelQueue.Add(this);
+        }
+
+        /// <summary>
+        ///     Del() the object, cleaning up its variables and refs to minimize size until the .NET GC collects it.
+        /// </summary>
+        /// <param name="possiblyThreaded">If true, Delete() will be defensive and assume it may have been called from another thread by .NET</param>
+        public void Delete(bool possiblyThreaded = false) {
             if (Deleted)
                 return;
 
-            if (TryGetProc("Del", out var delProc))
-                DreamThread.Run(delProc, this, null);
+            if (TryGetProc("Del", out var delProc)) {
+                // SAFETY: See associated comment in Datum.dm. This relies on the invariant that this proc is in a
+                //         thread-safe subset of DM (if such a thing exists) or empty. Currently, it is empty.
+                var datumBaseProc = delProc.OwningType == ObjectTree.Datum;
+                if (possiblyThreaded && !datumBaseProc) {
+                    EnterIntoDelQueue();
+                    return; //Whoops, cannot thread.
+                } else if (!datumBaseProc) {
+                    DreamThread.Run(delProc, this, null);
+                }
+            }
+
+            HandleDeletion(possiblyThreaded);
+        }
 
-            HandleDeletion();
+        ~DreamObject() {
+            // Softdel, possibly.
+            Delete(true);
         }
 
         public bool IsSubtypeOf(TreeEntry ancestor) {

OpenDreamRuntime/Objects/DreamObjectTree.cs

@@ -127,6 +127,8 @@ public IEnumerable<TreeEntry> GetAllDescendants(TreeEntry treeEntry) {
             IEnumerator<TreeEntry> typeChildren = GetAllDescendants(type).GetEnumerator();
 
             while (typeChildren.MoveNext()) yield return typeChildren.Current;
+
+            typeChildren.Dispose();
         }
     }
 

OpenDreamRuntime/Objects/Types/DreamObjectAtom.cs

@@ -27,10 +27,16 @@ public override void Initialize(DreamProcArguments args) {
         ObjectDefinition.Variables["desc"].TryGetValueAsString(out Desc);
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: RemoveAtom is not threadsafe.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         AtomManager.RemoveAtom(this);
 
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     protected override bool TryGetVar(string varName, out DreamValue value) {

OpenDreamRuntime/Objects/Types/DreamObjectClient.cs

@@ -25,11 +25,17 @@ public DreamObjectClient(DreamObjectDefinition objectDefinition, DreamConnection
         View = DreamManager.WorldInstance.DefaultView;
     }
 
-    protected override void HandleDeletion() {
-        Connection.Session?.ConnectedClient.Disconnect("Your client object was deleted");
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Client hashset is not threadsafe, this is not a hot path so no reason to change this.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
+        Connection.Session?.Channel.Disconnect("Your client object was deleted");
         DreamManager.Clients.Remove(this);
 
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     protected override bool TryGetVar(string varName, out DreamValue value) {

OpenDreamRuntime/Objects/Types/DreamObjectFilter.cs

@@ -11,8 +11,14 @@ public sealed class DreamObjectFilter(DreamObjectDefinition objectDefinition) :
 
     public DreamFilter Filter;
 
-    protected override void HandleDeletion() {
-        base.HandleDeletion();
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Attachment dictionary is not threadsafe, no reason to change this.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
+        base.HandleDeletion(possiblyThreaded);
 
         FilterAttachedTo.Remove(Filter);
     }

OpenDreamRuntime/Objects/Types/DreamObjectImage.cs

@@ -239,10 +239,16 @@
         return _entity;
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Deleting entities is not threadsafe.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         if(_entity != EntityUid.Invalid) {
             EntityManager.DeleteEntity(_entity);
         }
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 }

OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs

@@ -56,11 +56,17 @@ public override void Initialize(DreamProcArguments args) {
         SetLoc(loc); //loc is set before /New() is ever called
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Deleting entities is not threadsafe.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
         WalkManager.StopWalks(this);
         AtomManager.DeleteMovableEntity(this);
 
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     protected override bool TryGetVar(string varName, out DreamValue value) {

OpenDreamRuntime/Objects/Types/DreamObjectSavefile.cs

@@ -124,9 +124,22 @@
         Savefiles.Add(this);
     }
 
-    protected override void HandleDeletion() {
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Close() is not threadsafe and doesn't have reason to be.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
+        if (Deleted)
+            return;
+
+        if (Resource is null)
+            return; // Seemingly a long-standing issue, I don't know how to fix this, and it only now appears due to the fact objects always Del() now.
+                    // Why we can get here? who knows lol
+
         Close();
-        base.HandleDeletion();
+        base.HandleDeletion(possiblyThreaded);
     }
 
     protected override bool TryGetVar(string varName, out DreamValue value) {

OpenDreamRuntime/Objects/Types/DreamObjectWorld.cs

@@ -85,8 +85,14 @@ public DreamObjectWorld(DreamObjectDefinition objectDefinition) :
             new DreamValue(ObjectTree.CreateList());
     }
 
-    protected override void HandleDeletion() {
-        base.HandleDeletion();
+    protected override void HandleDeletion(bool possiblyThreaded) {
+        // SAFETY: Server shutdown is, spoiler, not threadsafe.
+        if (possiblyThreaded) {
+            EnterIntoDelQueue();
+            return;
+        }
+
+        base.HandleDeletion(possiblyThreaded);
 
         _server.Shutdown("world was deleted");
     }

OpenDreamRuntime/Procs/DMOpcodeHandlers.cs

@@ -143,7 +143,7 @@ public static ProcStatus CreateTypeEnumerator(DMProcState state) {
             }
 
             if (type.ObjectDefinition.IsSubtypeOf(state.Proc.ObjectTree.Datum)) {
-                state.Enumerators[enumeratorId] = new DreamObjectEnumerator(state.DreamManager.Datums, type);
+                state.Enumerators[enumeratorId] = new DreamObjectEnumerator(state.DreamManager.IterateDatums(), type);
                 return ProcStatus.Continue;
             }