Are there any negative side effects to setting hooks this way?

[dusk]

I am about to write my first plugin. After reading over the documentation, I decided to try and be creative with the syntax for setting hooks, because I felt like the "official" recommended way was going to be really difficult for me to read and follow. So I tried setting hooks this way, outside of the Initialize function and using anonymous functions for the function argument, and behold, it works

Screenshot: Show

One thing I noticed was that code outside of any function runs before the Initialize function is called. So I'm wondering, are there any negative side effects to connecting these events before the plugin has been technically initialized?

[xoft]

Yes, it is possible to register hooks that way, and your insight that the registrations execute before the Initialize function is correct, and is central to Lua. When (any) Lua interpreter reads code, it first parses it into an internal representation, and then executes *all* of it. It doesn't mean it calls all functions, but rather, the execution sees "function Initialize() ... end" and knows "aha, this code creates a new function and names it Initialize", so it internally registers a new function of the name and points it to the block of executable code. Then it sees some function calls to AddHook, so it does those immediately.

There might be some negative points to registering hooks this way. I'll describe how Cuberite loads plugins, and you'll see them.

First Cuberite makes a list of all .lua files in the plugin folder, and it sorts the list by name [*]. Then, it loads each file (load = reading and executing the code, as described above). Then it asks Lua to execute the global function Initialize. So what could go wrong? As long as your plugin is within just 1 file, not much, really. But consider this:

File a.lua:

-- At global scope, outside all functions:
cPluginManager.AddHook(cPluginManager.HOOK_PLAYER_MOVING, OnPlayerMoving)

File b.lua:

function OnPlayerMoving(...)
  -- The actual implementation
end

In this case, the plugin is loaded, file-by-file, so first the file a.lua is loaded and executed first. The hook registration fails, because OnPlayerMoving is unknown at this point (thus nil, by Lua definition). If the file names were swapped, then everything would work just fine, which is quite unfortunate, because it hides the possible problem until someone refactors the code and renames the files again. For this reason, we do encourage people to use the Initialize function as the place where all initialization should be done - when Cuberite calls that function, you can be sure that whatever the file load order, your global symbols are already defined correctly.


[*]: Special exception: the Info.lua file is always loaded as the last one, so that the function references inside it work properly.

[dusk]

Yes, it is possible to register hooks that way, and your [...]

Thanks for the detailed description
I'm really fond of Cuberite, it's a wonderful project, and I can see that it has passionate devs.

I can see where the problem would arise. Would I be correct in assuming that setting hooks outside of any function, in the global scope, would always be safe from this problem only provided that an anonymous function is passed as the function argument? e.g.:

cPluginManager.AddHook(cPluginManager.HOOK_WHATEVER, function(...)
    --[[...]]
end)

Instead of:

cPluginManager.AddHook(cPluginManager.HOOK_WHATEVER, HookWhateverHandler)

[NiLSPACE]

If you use anonymous functions I don't see how it could go wrong.

[xoft]

An anonymous function will be alright. A global function will be alright as long as you know it will be defined before the registration (i.e. same file, function above registration). All other usages should be considered unsafe and avoided.

[xoft]

By the way, the fact that hook registrations outside of function calls work is a side effect of what we've been trying to do once, a feature that would allow multiple plugins to be merged into a single plugin, simply by copying the files over (assuming that the filenames had no conflict). This was abandoned, mostly because of the Info.lua special file and because it didn't bring as many benefits as expected, but the underlying features were left over.

[NiLSPACE]

An anonymous function will be alright. A global function will be alright as long as you know it will be defined before the registration (i.e. same file, function above registration). All other usages should be considered unsafe and avoided.

Didn't we also have a problem previously where an OS with case-sensitive filenames ordered the files different compared to an OS with case-insensitive filenames or has that been fixed ?

[xoft]

Still there, Cuberite uses case-sensitive sort, but if the OS / filesystem doesn't preserve the filename case, it's a problem.

And I'm not intending to fix that, because case-insensitivizing is a terrible amount of work when coupled with internationalization.

[TC1061]

There is another solution. You can store all callbacks in a hooks table, and register all callbacks from this table, for example:

Code:

hooks={}
function hooks.tick(e,d) -- Will be registered as HOOK_TICK hook.
  -- Do something cool
  return ... -- Return something cool
end
function Initialize(Plugin)
  Plugin:SetName("SomeCoolPlugin")
  Plugin:SetVersion(1)
  if hooks then
    for k,v in pairs(hooks) do
      cPluginManager.AddHook(cPluginManager["HOOK_"..string.upper(k)], v)
    end
  end
  PLUGIN = Plugin -- NOTE: only needed if you want OnDisable() to use GetName() or something like that
  if commands then
    for k,v in pairs(commands) do
      cPluginManager.BindCommand(unpack(v))
    end
  end
  LOG("Initialised " .. Plugin:GetName() .. " v." .. Plugin:GetVersion())
return true
end

function OnDisable()
  LOG(PLUGIN:GetName() .. " is shutting down...")
end

[NiLSPACE]

That's a neat way of storing the hooks, but I'd probably use something like this instead:

Code:

local hooks = {
    [cPluginManager.HOOK_TICK] = function(TimeDelta)
    end
}


function Initialize(Plugin)
    Plugin:SetName("SomeCoolPlugin")
    Plugin:SetVersion(1)
    if hooks then
        for hook, callback in pairs(hooks) do
            cPluginManager:AddHook(hook, callback)
        end
    end
    
    return true;
end