Custom Event Module

[kingdom5]

With the changes to coroutine.yield it is now possible to yield a thread and resume it upon another action being made.

This is the first draft of my custom event module so I expect a lot of of things to change. The goal of this module is to closely simulate the functionality of value holders and bindable events (will look at adding in bindable functions at a later stage after some code refactoring).

This is shown below in the gif.

The module can be here (test script and API included).

Here are some of my concerns.

• Tables are not cloned within a event call meaning any edits will roll over to the next function in the call list. Cloning a table for each function call would be slow and has its limits.

• Each function in the call list is not a new thread. Any yield will delay the next function call in the call list.

• Upon using Destroy all functions in the call list are disconnected and all waiting threads are resumed with the current value (for value objects) or nil (for event objects).

Change Log 1.3

• wait for event/function object to exist.

Change Log 1.2

• FunctionObjects added. Errors will not stop calling code but will have no return values.
• Error format changed to better mimic Roblox errors.

Change Log 1.1

• All functions in the call list are new threads.
• Thread errors are printed to the output in red using TestService:Error and do not effect the calling thread in any way (smaller issue with yield then error printing to the output see test 4).
• Duplicate code removed.

Thank you for taking the time to look through this. Any feedback about this module is welcome.

[Avigant]

Why don’t you call coroutine.wrap()?

Instead of:

function obj:Fire(...)
	if disable then return error("Cannot use Fire after Destroy function call.") end
	for i=1, #callList do callList[i][1](...) end
	for i=1, #waitList do coroutine.resume(waitList[i], ...) end
end

Why not just:

function obj:Fire(...)
	if disable then return error("Cannot use Fire after Destroy function call.") end
	for i=1, #callList do coroutine.wrap(function() callList[i][1](...) end)() end
end

After the callList[i][1](...) call is done, you can compare the number of completed listeners to the total amount of listeners, and if they are equal, you can resume all the threads that are waiting on the event.

Tables should be copied for fires. This will prevent uncommon but hard to solve bugs when you modify the listener table while it is being iterated over. You can optimize this by only deep copying the listener table if it has been changed since the last deep copy, otherwise use the last deep copy. This should be efficient enough in many cases.

I must confess I do not see the use in destroying an event object. When is this a useful operation? This just seems like it’s going to cause bugs.

As for BindableFunctions, there’s really no reason to add them at all, because they predate ModuleScripts, which have superceded them completely.

I’ve written a BindableEvent-like replacement myself before a number of months ago, the difference is that in my implementation the fire call yields until all listeners have been completed (listeners can make asynchronous calls, this will not delay the total runtime of the fire unnecessarily):

local message_broadcaster = {}

local services = {
    replicated_storage = game:GetService("ReplicatedStorage"),
}

local core_import_root = services.replicated_storage.core
local imports = {
    core = {
        table_utility = require(core_import_root.table_utility),
    },
}

local channel_data = {}

local function create_channel(name)
    local new_data_of_channel = {
        listener_amount = 0,
        listeners = {},
    }

    channel_data[name] = new_data_of_channel

    return new_data_of_channel
end

local function destroy_channel(name)
    channel_data[name] = nil
end

function message_broadcaster.subscribe(channel_name, listener)
    local data_of_named_channel = channel_data[channel_name]

    if not data_of_named_channel then
        data_of_named_channel = create_channel(channel_name)
    end

    data_of_named_channel.listeners[listener] = true
    data_of_named_channel.listener_amount = data_of_named_channel.listener_amount + 1
end

function message_broadcaster.send(channel_name, ...)
    local data_of_named_channel = channel_data[channel_name]

    if not data_of_named_channel then
        return
    end

    local packed_message = {...}
    local listeners_copy = imports.core.table_utility.get_deep_copy(data_of_named_channel.listeners)
    local listener_amount_copy = data_of_named_channel.listener_amount

    local completion_event = Instance.new("BindableEvent")
    local completed_listener_amount = 0
    local is_broadcast_completed = false

    local function handle_broadcast_completion()
        is_broadcast_completed = true
    end

    completion_event.Event:Connect(handle_broadcast_completion)

    for current_listener in pairs(listeners_copy) do
        local function current_listener_wrapper()
            current_listener(unpack(packed_message))

            completed_listener_amount = completed_listener_amount + 1

            if completed_listener_amount == listener_amount_copy then
                completion_event:Fire()
            end
        end

        local current_listener_thread = coroutine.wrap(current_listener_wrapper)

        current_listener_thread()
    end

    if not is_broadcast_completed then
        completion_event.Event:Wait()
    end

    completion_event:Destroy()
end

function message_broadcaster.unsubscribe(channel_name, listener)
    local data_of_named_channel = channel_data[channel_name]

    data_of_named_channel.listeners[listener] = nil
    data_of_named_channel.listener_amount = data_of_named_channel.listener_amount - 1

    if data_of_named_channel.listener_amount > 0 then
        return
    end

    destroy_channel(channel_name)
end

return message_broadcaster

I think it’s much simpler just to have a simple pub-sub API like mine, regardless of the implementation details, rather than introducing OOP without any real benefit. I can’t see a gain in your approach vs. just sending messages on a channel specified by a string name.

table_utility module relevant to my code:

local table_utility = {}

local function get_deep_copy(table_to_copy, previously_copied_tables)
    local copy_of_table = {}

    if not previously_copied_tables then
        previously_copied_tables = {}
    end

    local previous_copy_of_table = previously_copied_tables[table_to_copy]

    if previous_copy_of_table then
        return previous_copy_of_table
    end

    previously_copied_tables[table_to_copy] = copy_of_table

    local metatable_to_copy = getmetatable(table_to_copy)

    if metatable_to_copy then
        local copy_of_metatable = nil
        local previous_copy_of_metatable = previously_copied_tables[metatable_to_copy]
        
        if previous_copy_of_metatable then
            copy_of_metatable = previous_copy_of_metatable
        else
            copy_of_metatable = get_deep_copy(metatable_to_copy, previously_copied_tables)
        end

        setmetatable(copy_of_table, copy_of_metatable)
    end
        
    for current_key, current_value in pairs(table_to_copy) do
        if type(current_key) == "table" then
            current_key = get_deep_copy(current_key, previously_copied_tables)
        end

        if type(current_value) == "table" then
            current_value = get_deep_copy(current_value, previously_copied_tables)
        end

        copy_of_table[current_key] = current_value
    end

    return copy_of_table
end

function table_utility.get_deep_copy(table_to_copy)
    return get_deep_copy(table_to_copy)
end

return table_utility

[sleitnick]

Be sure to use assert to wrap your coroutine.resume calls! coroutine.resume runs in protected mode by default (i.e. like pcall). Thus, if anything errors within your resume, it will silently fail.

Simply wrap it with assert to avoid this:

assert(coroutine.resume(...))

Edit: In regards to @Avigant’s post, coroutine.wrap will show errors; assert is not necessary with wrap.

[kingdom5]

I will create each call as a new thread but I do not see why I would wait for each function in the callList to finish before resuming the waiting threads.

It is more standard practice for all arguments in a function to be a copy. In Roblox I have rarely seen code that creates a copy of a table before passing it onto a function. Both of these approaches have their pros and cons but I still think it should be down to each individual function to maintain the integrity of the table.

The Destroy function should be used with care which is why it is setup to error upon calling a function in this object. The idea behind this is to release any resources for the object as it is looking after its own state and disregarding the state of any other code. The other code should be changed if this causes things to break.

I have never liked the idea that Roblox allows you to change the function at any time for a BindableFunction/RemoteFunction so the only goal of adding this in would be to make sure the function cannot be changed after it has been set.

In the module using OOP is done to ensure consistency of types.

[kingdom5]

I often forget about using assert in Lua and end up just using error. It is also very nice that the return values of coroutine.resume fits assert.

I will review this in the code.