+ Reply to Thread
Results 1 to 14 of 14
Like Tree3Likes
  • 1 Post By Imhothar
  • 1 Post By Naifu
  • 1 Post By Imhothar

Thread: Why table.remove(Event.Something, myIndex) is bad

  1. #1
    Plane Walker Imhothar's Avatar
    Join Date
    Feb 2012
    Posts
    439

    Default Why table.remove(Event.Something, myIndex) is bad

    While going through some addons to look how they do things I noticed that some have the habit of enabling/disabling event hooks by manipulating the event table.

    This is bad!

    First, one can see the event index to be retrieved like this:
    Code:
    index = table.insert(Event.Something, { handler, ... })
    The first false assumtion here is, that table.insert returns a value. It does NOT!
    index is nil. This may be a source of bugs.
    You can verify this behavior with
    Code:
    /script print(table.insert({ }, "x"))
    The correct way to get the inserted index is
    Code:
    table.insert(Event.Something, { handler, ... })
    index = #Event.Something
    The #-operator gives the lengt of the array part of a table. As long as everyone is using table.insert this works fine.

    The real problem arises when they try to unhook the event by calling
    Code:
    table.remove(Event.Something, index)
    Now here is the problem:
    If index does not point to the last element in the event table, table.remove moves all table elements which are behind index down to fill the gap! If enyone tries to do the same more than once, they are most likely to delete event handlers of other addons which are completely unrelated! You now violated the stored table index values of all other addons which were loaded after yours!

    Note also, that if you tried to get the index as shown in the first example by reading the result of table.insert the call to table.remove does something evil:
    Code:
    table.remove(Event.Something, nil)
    removes the last element from the table, which can be an event handler of whatever addon has registered for the evetn after yours.

    A better way to disable your event hook is
    Code:
    Event.Something[index][1] = function() end
    By assigning an empty function as event hook you basically disabled it. To re-enable your hook simply do
    Code:
    Event.Something[index][1] = handler
    An even better way it to store a reference to the table entry and access that.
    Code:
    table.insert(Event.Something, { hook, ...})
    entry = Event.Something[#Event.Something]
    And to disable it:
    Code:
    entry[1] = function() end
    That makes your own addon resistent to being screwed up if someone else insists on calling table.remove.

    Tell all your friends!
    Last edited by Imhothar; 03-10-2012 at 01:49 PM.

  2. #2
    RIFT Community Ambassador the_real_seebs's Avatar
    Join Date
    Jan 2011
    Posts
    16,859

    Default

    Interesting. I've been using the .remove, but I've been doing it only after searching the table for my events. I don't assume that my place in the table will remain fixed one I've transferred control elsewhere.
    You can play WoW in any MMO. You don't have to play WoW in RIFT. Oh, and no, RIFT is not a WoW clone. Not having fun any more? Learn to play, noob! I don't speak for Riftui, but I moderate stuff there. Just came back? Welcome back! Here's what's changed. (Updated for 2.5!)

  3. #3
    Plane Walker Imhothar's Avatar
    Join Date
    Feb 2012
    Posts
    439

    Default

    The problem is if other addons save their index and assume it is static hell breaks lose after your table.remove.

    It can be a real pain for these authors to find the source of such a bug because it is caused by a different addon uncontrollably.

  4. #4
    RIFT Community Ambassador the_real_seebs's Avatar
    Join Date
    Jan 2011
    Posts
    16,859

    Default

    You have a point. On the one hand, I want to avoid the presumably non-zero cost of being called every update if I'm not doing anything, but... hmm.
    You can play WoW in any MMO. You don't have to play WoW in RIFT. Oh, and no, RIFT is not a WoW clone. Not having fun any more? Learn to play, noob! I don't speak for Riftui, but I moderate stuff there. Just came back? Welcome back! Here's what's changed. (Updated for 2.5!)

  5. #5
    Plane Walker Imhothar's Avatar
    Join Date
    Feb 2012
    Posts
    439

    Default

    For anyone who has problems imagining this, here is a step-by-step demonstration:

    We have three addons isntalled: A, B and C.
    Addons A and B want to dynamically hook and unhook Event.Buff.Add, a quite important event for boss addons.
    Addon C only hooks to Event.Buff.Add statically only once.

    So after all the addons have hooked the event they have stored the indices
    Code:
    indexA = 1
    indexB = 2
    indexC = 3
    and Event.Buff.Add looks like this (simplified)
    Code:
    { 1 = hookA, 2 = hookB, 3 = hookC }
    Now, at some point addon A decides it no longer needs its event, so calls table.remove(Event.Buff.Add, indexA).
    This results in the following Event.Buff.Add table:
    Code:
    { 1 = hookB, 2 = hookC }
    Now addon B decides it doesn't need its event anymore and calls table.remove(Event.Buff.Add, indexB).
    But because indexB is stil 2 the Event.Buff.Add table changes to:
    Code:
    { 1 = hookB }
    I hope everyone can see the dilemma now. Addon B was not unhooked as it wanted to and Addon C has been disabled unwillingly by a different addon.

  6. #6
    Champion
    Join Date
    Jun 2011
    Posts
    561

    Default

    I'm using this code in my addons (actually in my nkTools lib).

    Code:
    function nkTools.events.remove (eventTable, eventID)
    
    	for key, details in pairs (eventTable) do
    		if details[3] == eventID then
    			table.remove(eventTable, key)
    		end
    	end
    
    end
    Exmaple of usage:

    Code:
    table.insert(Event.System.Update.Begin, {myAddon.events.updateHandler, "myAddon", "myEventID"})
    
    nkTools.events.remove (Event.System.Update.Begin, "myEventID")
    Works quite well for me this way. No problem with the table index as I look for the exact event to remove.

    Very small potential of conflict if another addon would inject or remove an event at exactly that milisecond it takes the api to do the table.remove. Not even sure if that would count in practise.

    Cheers
    N.

  7. #7
    Plane Walker Imhothar's Avatar
    Join Date
    Feb 2012
    Posts
    439

    Default

    Quote Originally Posted by Naifu View Post
    I'm using this code in my addons (actually in my nkTools lib).

    Code:
    function nkTools.events.remove (eventTable, eventID)
    
    	for key, details in pairs (eventTable) do
    		if details[3] == eventID then
    			table.remove(eventTable, key)
    		end
    	end
    
    end
    Exmaple of usage:

    Code:
    table.insert(Event.System.Update.Begin, {myAddon.events.updateHandler, "myAddon", "myEventID"})
    
    nkTools.events.remove (Event.System.Update.Begin, "myEventID")
    Works quite well for me this way. No problem with the table index as I look for the exact event to remove.

    Very small potential of conflict if another addon would inject or remove an event at exactly that milisecond it takes the api to do the table.remove. Not even sure if that would count in practise.

    Cheers
    N.
    Unfortunately you do not see the entire message: Performing table.remove itself is dangerous as long as not every addon (and I mean without exception) does the same as you and does not store an index into the event table. Only then does calling table.remove work. And only then does it not screw other addons.

    So either nobody uses table.remove on events or nobody stores index values for event tables. Otherwise Addons will screw up each other.

    As a side note: The Lua code does not run multithreaded in parallel so you can rest assured there are no such race conditions as you described.
    Last edited by Imhothar; 03-10-2012 at 11:06 AM.

  8. #8
    Rift Disciple
    Join Date
    Feb 2011
    Posts
    129

    Default

    Quote Originally Posted by Imhothar View Post
    Note also, that if you tried to get the index as shown in the first example by reading the result of table.insert the call to table.remove does nothing because
    Code:
    table.remove(Event.Something, nil)
    does nothing.
    But it does do something, at least in my case it removes the function from the event.

    Code:
    table.remove(Event.System.Update.Begin,nil);
    With it, the function stops getting called, without it it gets called indefinitely.
    Last edited by Xioden; 03-10-2012 at 12:40 PM.

  9. #9
    Plane Walker Imhothar's Avatar
    Join Date
    Feb 2012
    Posts
    439

    Default

    Quote Originally Posted by Xioden View Post
    But it does do something, at least in my case it removes the function from the event.

    Code:
    table.remove(Event.System.Update.Begin,nil);
    With it, the function stops getting called, without it it gets called indefinitely.
    Oh yes, you are right. table.remove with nil as second argument removes the last element from the table. I forgot the second argument is optional and updated my first post.

    So in fact you were lucky until now because your event handler happened to be the last in the table.
    Last edited by Imhothar; 03-10-2012 at 12:48 PM.

  10. #10
    Rift Disciple
    Join Date
    Feb 2011
    Posts
    129

    Default

    Quote Originally Posted by Imhothar View Post
    Oh yes, you are right. table.remove with nil as second argument removes the last element from the table. I forgot the second argument is optional and updated my first post.

    So in fact you were lucky until now because your event handler happened to be the last in the table.
    So wouldn't this be the way to remove it then?

    Code:
    --inserting
    event_id={foo_function, "foo", "foo description" }
    table.insert(Event.System.Update.Begin,event_id)
    
    --removing
    for id, eventData in ipairs(Event.System.Update.Begin) do
    	if eventData == event_id then
    		table.remove(Event.System.Update.Begin, id)
    		break
    	end
    end
    Last edited by Xioden; 03-10-2012 at 01:05 PM.

  11. #11
    Plane Walker Imhothar's Avatar
    Join Date
    Feb 2012
    Posts
    439

    Default

    In theory, yes.
    But it will break other Addons if they store the index themselves for whatever reason.
    The safest way is to assign the entry in the event table a no-op.

    That is actually the message of my post:
    Do not remove elements from event tables unless we can force all addon developers to get rid of storing table indices.

    Using the table index is unfortunately the obvious and easiest way of accessing the event table, so I fear many people are going to do just that.

  12. #12
    Champion
    Join Date
    Jun 2011
    Posts
    561

    Default

    There will always be ways to screw up other addons by accident. The best way would be an addition to the api to safely remove events. I agree this would be needed asap. Dynamicly adding and removing system events is very much needed imho.

    But tbh anyone storing the index and removing events based on this should really think about his development skills. If one doesn't repcognize the problem around this should stop working on addons at all.

    Cheers
    N.
    Last edited by Naifu; 03-10-2012 at 01:41 PM.

  13. #13
    Plane Walker Imhothar's Avatar
    Join Date
    Feb 2012
    Posts
    439

    Default

    Quote Originally Posted by Naifu View Post
    There will always be ways to screw up other addons by accident. The best way would be an addition to the api to safely remove events
    Agreed, but this issue in particular is quite nasty as it is nondeterministic, can almost impossible to debug if you don't have exactly the same addons as the users reporing errors.

    I also agree an API method would be nice to have, but considering we register for events with table.insert and not an API call, I assume it gets a low priority because Lua itself offers the tools to avoid it. My suggestion is not to rely on any table index but store a reference to the table entry itself (the { hook, identifier, name } thing) and setting the handler to an empty function instead of using table.remove.

    That makes your own addon resistent to being screwed up.
    Last edited by Imhothar; 03-10-2012 at 01:47 PM.

  14. #14
    Telaran
    Join Date
    Mar 2012
    Posts
    83

    Default

    Good point.

    To answer the question on how do you properly remove the event? Don't.
    If you have a part of your code that is supposed to only react to certain events under some special condition, check that condition at the top of your function. Example:

    Code:
    function onUpdate()
      if (ready) then
        ...
      end
    end
    Yes you do waste a lot of calls to that function, but if that worries you, then in most cases you are either hooking the wrong event (frame.update is evil!) or you could probably remove such calls completely by changing your code structure. However if you must hook something like frame.update, keep in mind how long it takes to check a boolean variable compared to the rendering of the entire screen and its probably thousand of polygons, textures, shaders and so on.

    The gain might not be worth the time it takes to actually write complex event removal code. ;)

+ Reply to Thread

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts