Events.Xyz.Remove() is still buggy for anonymous functions/closures

[whoward69]

FYI

self.moveHandler = function(iPlayer, iUnit, iX, iY)
  -- Do something

  -- Remove myself
  -- This will "remove" the handler, but not actually stop calling it!
  -- And CTD on the second call
  GameEvents.UnitSetXY.Remove(self.moveHandler)
end
GameEvents.UnitSetXY.Add(self.moveHandler)

 

[Hambil]

So, cleaning up after yourself is bad. They are teaching us such wonderful programming habbits!

 

[whoward69]

Well it never used to work at all. Just to be clear, its only seems to be anonymous functions, the following still works

function OnMove(iPlayer, iUnit, iX, iY)
  -- Do something

  -- Remove myself, this works
  GameEvents.UnitSetXY.Remove(OnMove)
end
GameEvents.UnitSetXY.Add(OnMove)

 

[Hambil]

Ah, okay, the second is what I was doing anyway. As long as that works. Thanks for the clarification.

 

[Pazyryk]

I believe you are running into Lua compilation problem. The issue is addressed here as it affects recursion. Basically,

function DoSomething()
	if (something) do

	else
		return DoSomething()
	end
end

works, but

local DoSomething = function()
	if (something) do

	else
		return DoSomething()
	end
end

and

function MyFunctions.DoSomething()
	if (something) do

	else
		return MyFunctions.DoSomething()
	end
end

do not work.

The reason the 2nd and 3rd don't work is that DoSomething (in the 2nd code) and MyFunctions.DoSomething (in the 3rd code) are both nil at the time that the return line is compiled. However, the 1st works because DoSomething is interpreted at compilation as a valid global (doesn't matter what's in the global until the function actually runs). You can fix code 2 by defining the local variable first and then assigning the function to it. Fix code 3 by defining the function and then assigning it to the table element.

It looks to me that you are doing the same thing: GameEvents.UnitSetXY.Remove(self.moveHandler) is the same as GameEvents.UnitSetXY.Remove(nil) when that line is compiled.

 

[whoward69]

I shall keep digging as print(self.moveHandler) just before the Remove() spits out the expected function pointer and not nil

But certainly an interesting page

And probably better to err on the safe side.

Edit and now I'm just totally confused!

The cut down actual code

Watcher = {
  Add = function(self)
    self.moveHandler = function(iPlayer, iUnit, iX, iY)
      if (self.iUnit == iUnit and self.iPlayer == iPlayer) then
        self:MoveTo(iX, iY)
      end
    end
    GameEvents.UnitSetXY.Add(self.moveHandler)
  end,

  MoveTo = function(self, iX, iY)
    print(string.format("I just moved to (%i, %i)", iX, iY))

    -- Stop watching our movement, this causes a CTD
    GameEvents.UnitSetXY.Remove(self.moveHandler)
  end,
}

if self.moveHandler is nil in MoveTo, then why is self:MoveTo() (which is just self.MoveTo(self)) not also nil in Add

My brain hurts!

More edits: And from Lua/SciTe - which mimics exactly what I'm seeing in FireTuner just before the Remove goes CTD

Watcher = {
  Add = function(self)
    self.moveHandler = function(iPlayer, iUnit, iX, iY)
      if (self.iUnit == iUnit and self.iPlayer == iPlayer) then
        self:MoveTo(iX, iY)
      end
    end
    print("Add", self.moveHandler)
  end,

  MoveTo = function(self, iX, iY)
    print(string.format("I just moved to (%i, %i)", iX, iY))

    -- Stop watching our movement, this causes a CTD
    print("Remove", self.moveHandler)
  end,
}

Watcher.iPlayer = 0
Watcher.iUnit = 1
Watcher:Add()

Watcher.moveHandler(0, 1, 12, 15)


>lua -e "io.stdout:setvbuf 'no'" "confused.lua" 
Add	function: 0073BE38
I just moved to (12, 15)
Remove	function: 0073BE38
>Exit code: 0

 

[Pazyryk]

Makes my head hurt too. The print seems to go against my theory though.

 

[whoward69]

I'm still inclined to label it as a "bug". When I first tried to use Remove() it wiped out all handlers in the (sandboxed) Lua instance - which as most people were only using one per UIAddin wasn't a "biggie". Shaun fixed that in one of the early patches, but I'm just wondering if there is some weird artifact hanging about

 

[whoward69]

Arrrggg!!!! And now it's crashing in different places due to execution order!

I have two of those anonymous functions tracking movement and removing one seems to be removing both, so it looks like a throw back to the earlier bug

 

[Pazyryk]

Are you sure you're thinking about lua functions the right way? I thought that they were all "anonymous" (as are tables). Syntax that seems to imply otherwise is just "syntactic sugar". The function (or prototype for the function) sits somewhere and is pointed to by globals, locals or table fields.

With tables, once all pointers to the table cease to exist, then Lua quietly garbage collects them. I wonder if that is happening for these functions that you are calling anonymous.

 

[whoward69]

Are you sure you're thinking about lua functions the right way? I thought that they were all "anonymous" (as are tables). Syntax that seems to imply otherwise is just "syntactic sugar". The function (or prototype for the function) sits somewhere and is pointed to by globals, locals or table fields.

With tables, once all pointers to the table cease to exist, then Lua quietly garbage collects them. I wonder if that is happening for these functions that you are calling anonymous.

I'm no longer sure I'm even thinking in any way - I started writing JavaScript in the middle of my Lua funtion last night and couldn't for the life of me figure out what was wrong with "if (a == 1 && b == 2) {" for about half and hour

The up side of all of this is that it forced me to completely rethink the design and I've re-written the movement tracking code and now only hook the event once, and never remove the handler, which is far more efficient.

I know that the table/object holding the listener isn't being garbage collected as it's still owned by the factory