howmanysmall / Janitor

Janitor library.
https://howmanysmall.github.io/Janitor/
MIT License
107 stars 17 forks source link

Avoid double-destroying instances #29

Closed Meta-Maxim closed 1 year ago

Meta-Maxim commented 1 year ago

May be another solution here, but my janitors run into errors when used in the following configuration:

  1. GuiModule creates a gui, adds to guiJanitor
  2. GuiModule creates a GuiComponent class, which uses LinkToInstance to destroy the component when the component's gui is destroyed.
  3. Gui class calls guiJanitor:Cleanup(), destroying the gui and the descendant component instance. This triggers the component class's LinkToInstance cleanup after the descendant component instance was already destroyed, resulting in an error.

As it's impossible to check if an instance has been destroyed already as opposed to just parented to nil, this change adds a pcall for only instance:Destroy() calls during cleanup.

Simple case:

local j = Janitor.new()
j:Add(workspace.p)
j:LinkToInstance(workspace.p)
workspace.p:Destroy() -- Ancestor destroyed maybe
--> `The Parent property of Workspace.p is locked`
howmanysmall commented 1 year ago

i actually almost added this for 1.15

howmanysmall commented 1 year ago

what i was thinking of doing is instead of this, making it a toggle option like i originally intended

Meta-Maxim commented 1 year ago

If you mean the component janitor can mark that instances may be destroyed for other reasons/errorOnCleaningDestroyed then that sounds better, the double-destruction error could be a useful for other designs

Meta-Maxim commented 1 year ago

@howmanysmall Added a toggle .SuppressInstanceReDestroy = false -- default

howmanysmall commented 1 year ago

You're gonna need to pull from the latest if you want this merged.

Meta-Maxim commented 1 year ago

howmanysmall commented 1 year ago

Okay, sorry about the delay. Pull again and I'll finally merge.

howmanysmall commented 1 year ago

There you go.