howmanysmall / Janitor

Janitor library.
https://howmanysmall.github.io/Janitor/
MIT License
105 stars 18 forks source link

Changed "pairs" loop to a "while get" loop #20

Closed codesenseAye closed 2 years ago

codesenseAye commented 2 years ago

If you add to the janitor in one of the cleanup functions you will create untracked connections/functions/etc

An example where the old janitor cleanup method would falter:

local obliterator = JANITOR.new()

obliterator:Add(function()
  print("cleanup 1") -- reached at first cleanup
  obliterator:Add(function()
      print("cleanup 2") -- reached at second cleanup
  end)
end)

obliterator:Cleanup()
obliterator:Cleanup()

PS: The maid module has this same "while get" loop (could be the reason why janitor is faster 🤷 )

codesenseAye commented 2 years ago

Also, I don't know if its my improper use or not but ...

local JANITOR = require(game.ReplicatedStorage.Janitor)
local jan = JANITOR.new()
local started = tick()

jan:Add(function()
    print("wiped")
end)

jan:Add(game["Run Service"].Stepped:Connect(function()
    if started + 0.5 < tick() then
        print('check')

        if jan and jan.Destroy then
            jan:Destroy()
        end
    end
end))

local newJan = JANITOR.new()
newJan:Add(jan)

jan:Add(function()
    newJan:Destroy()
end)

This also seems like a problem area for me. It will sometimes not remove the event connection and just keep firing. Removing the "CurrentlyCleaning" variable, putting the "[Object] = nil" setter before you run the functions (prevents continuous looping which is what I think the "CurrentlyCleaning" was suppose to do) seems to fix it -- leaving all of the other functionality intact.

howmanysmall commented 2 years ago

I can see that this is important, but I'd rather not create a new function every run. If you fix that I'd be happy to push it.

I'd much rather have it like this since it's a little more readable.

local function GetFenv(self)
    return function(): (any, StringOrTrue)
        for Object, MethodName in pairs(self) do
            if Object ~= IndicesReference then
                return Object, MethodName
            end
        end
    end
end
local Get = GetFenv(self)
-- use it like you normally would
howmanysmall commented 2 years ago

I'll actually make the changes myself. Thanks!