diff --git a/Content.Tests/DMTests.cs b/Content.Tests/DMTests.cs index c3a15159cb..c8de51f562 100644 --- a/Content.Tests/DMTests.cs +++ b/Content.Tests/DMTests.cs @@ -100,6 +100,9 @@ public void TestFiles(string sourceFile, DMTestFlags testFlags, int errorCode) { 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 { diff --git a/DMCompiler/DMStandard/Types/Datum.dm b/DMCompiler/DMStandard/Types/Datum.dm index 24effdd904..c14bfb2a23 100644 --- a/DMCompiler/DMStandard/Types/Datum.dm +++ b/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) diff --git a/OpenDreamRuntime/DreamManager.cs b/OpenDreamRuntime/DreamManager.cs index 66f596379a..558bdca152 100644 --- a/OpenDreamRuntime/DreamManager.cs +++ b/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,12 @@ public sealed partial class DreamManager { // Global state that may not really (really really) belong here public DreamValue[] Globals { get; set; } = Array.Empty(); public List GlobalNames { get; private set; } = new(); - public Dictionary ReferenceIDs { get; } = new(); - public Dictionary ReferenceIDsToDreamObject { get; } = new(); + public ConcurrentDictionary ReferenceIDsToDreamObject { get; } = new(); public HashSet Clients { get; set; } = new(); - public HashSet 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 Datums { get; set; } = new(); + public ConcurrentBag DelQueue = new(); public Random Random { get; set; } = new(); public Dictionary> Tags { get; set; } = new(); public DreamProc ImageConstructor, ImageFactoryProc; @@ -103,6 +108,13 @@ public void Update() { _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) { @@ -206,10 +218,14 @@ public string CreateRef(DreamValue value) { } } - 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 +257,26 @@ public string CreateRef(DreamValue value) { return $"[0x{((int) refType+idx):x}]"; } + /// + /// Iterates the list of datums + /// + /// Datum enumerater + /// As it's a convenient time, this will collect any dead datum refs as it finds them. + public IEnumerable 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; + } + } + public DreamValue LocateRef(string refString) { bool canBePointer = false; @@ -269,7 +305,7 @@ public DreamValue LocateRef(string refString) { 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; diff --git a/OpenDreamRuntime/Objects/DreamObject.cs b/OpenDreamRuntime/Objects/DreamObject.cs index 10025386e4..14d030aa3b 100644 --- a/OpenDreamRuntime/Objects/DreamObject.cs +++ b/OpenDreamRuntime/Objects/DreamObject.cs @@ -6,6 +6,7 @@ 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 +22,9 @@ public class DreamObject { [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,7 +85,7 @@ public DreamObject(DreamObjectDefinition objectDefinition) { // 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)); } } @@ -89,14 +93,12 @@ 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 +108,40 @@ protected virtual void HandleDeletion() { ObjectDefinition = null!; } - public void Delete() { + /// + /// 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. + /// + protected void EnterIntoDelQueue() { + DreamManager.DelQueue.Add(this); + } + + /// + /// Del() the object, cleaning up its variables and refs to minimize size until the .NET GC collects it. + /// + /// If true, Delete() will be defensive and assume it may have been called from another thread by .NET + 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 is DMProc {Bytecode.Length: 0}; + 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) { diff --git a/OpenDreamRuntime/Objects/DreamObjectTree.cs b/OpenDreamRuntime/Objects/DreamObjectTree.cs index b0a09d5827..f2492129f4 100644 --- a/OpenDreamRuntime/Objects/DreamObjectTree.cs +++ b/OpenDreamRuntime/Objects/DreamObjectTree.cs @@ -129,6 +129,8 @@ public IEnumerable GetAllDescendants(TreeEntry treeEntry) { IEnumerator typeChildren = GetAllDescendants(type).GetEnumerator(); while (typeChildren.MoveNext()) yield return typeChildren.Current; + + typeChildren.Dispose(); } } diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectAtom.cs b/OpenDreamRuntime/Objects/Types/DreamObjectAtom.cs index 8a0004c77d..df8754eb48 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectAtom.cs +++ b/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) { diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectClient.cs b/OpenDreamRuntime/Objects/Types/DreamObjectClient.cs index dcda7709ea..55b944f0fc 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectClient.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectClient.cs @@ -25,11 +25,17 @@ public DreamObjectClient(DreamObjectDefinition objectDefinition, DreamConnection View = DreamManager.WorldInstance.DefaultView; } - protected override void HandleDeletion() { + 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) { diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectDatabase.cs b/OpenDreamRuntime/Objects/Types/DreamObjectDatabase.cs index 0750868144..62ac253ada 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectDatabase.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectDatabase.cs @@ -23,9 +23,14 @@ public override void Initialize(DreamProcArguments args) { throw new DMCrashRuntime("Unable to open database."); } - protected override void HandleDeletion() { + protected override void HandleDeletion(bool possiblyThreaded) { + if (possiblyThreaded) { + EnterIntoDelQueue(); + return; + } + Close(); - base.HandleDeletion(); + base.HandleDeletion(possiblyThreaded); } /// diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectDatabaseQuery.cs b/OpenDreamRuntime/Objects/Types/DreamObjectDatabaseQuery.cs index 3d6fb1954c..2ab4e3f7ee 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectDatabaseQuery.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectDatabaseQuery.cs @@ -21,10 +21,15 @@ public override void Initialize(DreamProcArguments args) { SetupCommand(command, args.Values[1..]); } - protected override void HandleDeletion() { + protected override void HandleDeletion(bool possiblyThreaded) { + if (possiblyThreaded) { + EnterIntoDelQueue(); + return; + } + ClearCommand(); CloseReader(); - base.HandleDeletion(); + base.HandleDeletion(possiblyThreaded); } /// diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectFilter.cs b/OpenDreamRuntime/Objects/Types/DreamObjectFilter.cs index 5711960286..963b0dab74 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectFilter.cs +++ b/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); } diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs b/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs index b165e5aa4c..97cfae5a57 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectImage.cs @@ -239,10 +239,17 @@ public EntityUid GetEntity() { 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); } } diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs b/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs index 36c3ee56ab..9b38720942 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectMovable.cs +++ b/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) { diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectSavefile.cs b/OpenDreamRuntime/Objects/Types/DreamObjectSavefile.cs index ec18db03aa..81feba9d27 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectSavefile.cs +++ b/OpenDreamRuntime/Objects/Types/DreamObjectSavefile.cs @@ -40,7 +40,7 @@ public sealed class DreamObjectSavefile : DreamObject { /// /// Real savefile location on the host OS /// - public DreamResource Resource = default!; + public DreamResource? Resource; /// /// The current savefile data holder - the root of the savefile tree @@ -124,9 +124,22 @@ public override void Initialize(DreamProcArguments args) { 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) { @@ -138,7 +151,7 @@ protected override bool TryGetVar(string varName, out DreamValue value) { value = _eof ? DreamValue.True : DreamValue.False; return true; case "name": - value = new DreamValue(Resource.ResourcePath ?? "[no path]"); + value = new DreamValue(Resource?.ResourcePath ?? "[no path]"); return true; case "dir": value = new DreamValue(new SavefileDirList(ObjectTree.List.ObjectDefinition, this)); @@ -219,7 +232,7 @@ public static void FlushAllUpdates() { try { savefile.Flush(); } catch (Exception e) { - _sawmill.Error($"Error flushing savefile {savefile.Resource.ResourcePath}: {e}"); + _sawmill.Error($"Error flushing savefile {savefile.Resource!.ResourcePath}: {e}"); } } SavefilesToFlush.Clear(); @@ -227,14 +240,14 @@ public static void FlushAllUpdates() { public void Close() { Flush(); - if (_isTemporary && Resource.ResourcePath != null) { + if (_isTemporary && Resource?.ResourcePath != null) { File.Delete(Resource.ResourcePath); } //check to see if the file is still in use by another savefile datum - if(Resource.ResourcePath != null) { + if(Resource?.ResourcePath != null) { var fineToDelete = true; foreach (var savefile in Savefiles) { - if (savefile == this || savefile.Resource.ResourcePath != Resource.ResourcePath) continue; + if (savefile == this || savefile.Resource!.ResourcePath != Resource.ResourcePath) continue; fineToDelete = false; break; } @@ -246,8 +259,8 @@ public void Close() { } public void Flush() { - Resource.Clear(); - Resource.Output(new DreamValue(JsonSerializer.Serialize(_rootNode))); + Resource!.Clear(); + Resource!.Output(new DreamValue(JsonSerializer.Serialize(_rootNode))); } /// @@ -462,7 +475,7 @@ public SFDreamJsonValue SerializeDreamValue(DreamValue val, int objectCount = 0) //if this is a savefile, just return a filedata object with it encoded savefile.Flush(); //flush the savefile to make sure the backing resource is up to date return new SFDreamFileValue(){ - Name = savefile.Resource.ResourcePath, + Name = savefile.Resource!.ResourcePath, Ext = ".sav", Length = savefile.Resource.ResourceData!.Length, Crc32 = CalculateCrc32(savefile.Resource.ResourceData), diff --git a/OpenDreamRuntime/Objects/Types/DreamObjectWorld.cs b/OpenDreamRuntime/Objects/Types/DreamObjectWorld.cs index 833bb60a94..701e77a646 100644 --- a/OpenDreamRuntime/Objects/Types/DreamObjectWorld.cs +++ b/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"); } diff --git a/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs b/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs index 8e15ccf7bd..1ee37031ae 100644 --- a/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs +++ b/OpenDreamRuntime/Procs/DMOpcodeHandlers.cs @@ -144,7 +144,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; } diff --git a/OpenDreamRuntime/Procs/DMProc.cs b/OpenDreamRuntime/Procs/DMProc.cs index 5494ab551a..c454766cab 100644 --- a/OpenDreamRuntime/Procs/DMProc.cs +++ b/OpenDreamRuntime/Procs/DMProc.cs @@ -542,7 +542,7 @@ public override void Dispose() { _stackIndex = 0; _stack = null; - _dreamValuePool.Return(_localVariables); + _dreamValuePool.Return(_localVariables, true); _localVariables = null; _catchPosition.Clear(); diff --git a/OpenDreamRuntime/Util/WeakDreamRef.cs b/OpenDreamRuntime/Util/WeakDreamRef.cs new file mode 100644 index 0000000000..d5bf82e035 --- /dev/null +++ b/OpenDreamRuntime/Util/WeakDreamRef.cs @@ -0,0 +1,24 @@ +using OpenDreamRuntime.Objects; + +namespace OpenDreamRuntime.Util; + +public sealed class WeakDreamRef { + private readonly WeakReference _weakReference; + + public WeakDreamRef(DreamObject obj) => _weakReference = new WeakReference(obj); + + public DreamObject? Target { + get { + _weakReference.TryGetTarget(out var t); + return t; + } + } + + public override bool Equals(object? obj) { + return Target?.Equals(obj) ?? false; + } + + public override int GetHashCode() { + throw new NotSupportedException("WeakDreamRef cannot be used as a hashmap key or similar."); + } +}