fix(EventEmitter): properly deregister handlers, print errors that occur in handler (#373)

4 files changed, 111 insertions, 11 deletions

diff --git a/lua/mason-core/EventEmitter.lua b/lua/mason-core/EventEmitter.lua
index 5d7aeaa0..403628a0 100644
--- a/lua/mason-core/EventEmitter.lua
+++ b/lua/mason-core/EventEmitter.lua
@@ -1,3 +1,4 @@
+local log = require "mason-core.log"
 ---@class EventEmitter
 ---@field private __event_handlers table<any, table<fun(), fun()>>
 ---@field private __event_handlers_once table<any, table<fun(), fun()>>
@@ -14,18 +15,31 @@ function EventEmitter.init(obj)
 end
 
 ---@param event any
+---@param handler fun(...): any
+local function call_handler(event, handler, ...)
+    local ok, err = pcall(handler, ...)
+    if not ok then
+        vim.schedule(function()
+            log.fmt_warn("EventEmitter handler failed for event %s with error %s", event, err)
+            vim.api.nvim_err_writeln(err)
+        end)
+    end
+end
+
+---@param event any
 function EventEmitter:emit(event, ...)
     if self.__event_handlers[event] then
         for handler in pairs(self.__event_handlers[event]) do
-            pcall(handler, ...)
+            call_handler(event, handler, ...)
         end
     end
     if self.__event_handlers_once[event] then
         for handler in pairs(self.__event_handlers_once[event]) do
-            pcall(handler, ...)
-            self.__event_handlers_once[handler] = nil
+            call_handler(event, handler, ...)
+            self.__event_handlers_once[event][handler] = nil
         end
     end
+    return self
 end
 
 ---@param event any
@@ -35,6 +49,7 @@ function EventEmitter:on(event, handler)
         self.__event_handlers[event] = {}
     end
     self.__event_handlers[event][handler] = handler
+    return self
 end
 
 ---@param event any
@@ -44,16 +59,19 @@ function EventEmitter:once(event, handler)
         self.__event_handlers_once[event] = {}
     end
     self.__event_handlers_once[event][handler] = handler
+    return self
 end
 
 ---@param event any
 ---@param handler fun(payload: any)
 function EventEmitter:off(event, handler)
-    if vim.tbl_get(self.__event_handlers, { event, handler }) then
+    if self.__event_handlers[event] then
         self.__event_handlers[event][handler] = nil
-        return true
     end
-    return false
+    if self.__event_handlers_once[event] then
+        self.__event_handlers_once[event][handler] = nil
+    end
+    return self
 end
 
 ---@private
diff --git a/tests/mason-core/EventEmitter_spec.lua b/tests/mason-core/EventEmitter_spec.lua
new file mode 100644
index 00000000..4b9e3a6e
--- /dev/null
+++ b/tests/mason-core/EventEmitter_spec.lua
@@ -0,0 +1,72 @@
+local spy = require "luassert.spy"
+local match = require "luassert.match"
+
+local a = require "mason-core.async"
+local EventEmitter = require "mason-core.EventEmitter"
+
+describe("EventEmitter", function()
+    it("should call registered event handlers", function()
+        local emitter = EventEmitter.init(setmetatable({}, { __index = EventEmitter }))
+        local my_event_handler = spy.new()
+        emitter:on(
+            "my:event",
+            my_event_handler --[[@as fun()]]
+        )
+
+        emitter:emit("my:event", { table = "value" })
+        emitter:emit("my:event", 1337, 42)
+
+        assert.spy(my_event_handler).was_called(2)
+        assert.spy(my_event_handler).was_called_with(match.same { table = "value" })
+        assert.spy(my_event_handler).was_called_with(1337, 42)
+    end)
+
+    it("should call registered event handlers only once", function()
+        local emitter = EventEmitter.init(setmetatable({}, { __index = EventEmitter }))
+        local my_event_handler = spy.new()
+        emitter:once(
+            "my:event",
+            my_event_handler --[[@as fun()]]
+        )
+
+        emitter:emit("my:event", { table = "value" })
+        emitter:emit("my:event", 1337, 42)
+
+        assert.spy(my_event_handler).was_called(1)
+        assert.spy(my_event_handler).was_called_with(match.same { table = "value" })
+    end)
+
+    it("should remove registered event handlers", function()
+        local emitter = EventEmitter.init(setmetatable({}, { __index = EventEmitter }))
+        local my_event_handler = spy.new()
+        emitter:on(
+            "my:event",
+            my_event_handler --[[@as fun()]]
+        )
+        emitter:once(
+            "my:event",
+            my_event_handler --[[@as fun()]]
+        )
+
+        emitter:off(
+            "my:event",
+            my_event_handler --[[@as fun()]]
+        )
+
+        emitter:emit("my:event", { table = "value" })
+        assert.spy(my_event_handler).was_called(0)
+    end)
+
+    it(
+        "should print errors in handlers",
+        async_test(function()
+            spy.on(vim.api, "nvim_err_writeln")
+            local emitter = EventEmitter.init(setmetatable({}, { __index = EventEmitter }))
+            emitter:on("event", mockx.throws "My error.")
+            emitter:emit "event"
+            a.scheduler()
+            assert.spy(vim.api.nvim_err_writeln).was_called(1)
+            assert.spy(vim.api.nvim_err_writeln).was_called_with "My error."
+        end)
+    )
+end)
diff --git a/tests/mason-core/installer/handle_spec.lua b/tests/mason-core/installer/handle_spec.lua
index b1c75763..fb7b434e 100644
--- a/tests/mason-core/installer/handle_spec.lua
+++ b/tests/mason-core/installer/handle_spec.lua
@@ -7,9 +7,11 @@ describe("installer handle", function()
     it("should register spawn handle", function()
         local handle = InstallHandle.new(mock.new {})
         local spawn_handle_change_handler = spy.new()
-        handle:once("spawn_handles:change", spawn_handle_change_handler)
         local luv_handle = mock.new {}
+
+        handle:once("spawn_handles:change", spawn_handle_change_handler)
         handle:register_spawn_handle(luv_handle, 1337, "tar", { "-xvf", "file" })
+
         assert.same({
             uv_handle = luv_handle,
             pid = 1337,
@@ -22,11 +24,13 @@ describe("installer handle", function()
     it("should deregister spawn handle", function()
         local handle = InstallHandle.new(mock.new {})
         local spawn_handle_change_handler = spy.new()
-        handle:once("spawn_handles:change", spawn_handle_change_handler)
         local luv_handle1 = mock.new {}
         local luv_handle2 = mock.new {}
+
+        handle:on("spawn_handles:change", spawn_handle_change_handler)
         handle:register_spawn_handle(luv_handle1, 42, "curl", { "someurl" })
         handle:register_spawn_handle(luv_handle2, 1337, "tar", { "-xvf", "file" })
+
         assert.is_true(handle:deregister_spawn_handle(luv_handle1))
         assert.equals(1, #handle.spawn_handles)
         assert.same({
@@ -41,8 +45,10 @@ describe("installer handle", function()
     it("should change state", function()
         local handle = InstallHandle.new(mock.new {})
         local state_change_handler = spy.new()
+
         handle:once("state:change", state_change_handler)
         handle:set_state "QUEUED"
+
         assert.equals("QUEUED", handle.state)
         assert.spy(state_change_handler).was_called(1)
         assert.spy(state_change_handler).was_called_with("QUEUED", "IDLE")
@@ -54,11 +60,12 @@ describe("installer handle", function()
         local uv_handle = {}
         local handle = InstallHandle.new(mock.new {})
         local kill_handler = spy.new()
+
         handle:once("kill", kill_handler)
         handle.state = "ACTIVE"
         handle.spawn_handles = { { uv_handle = uv_handle } }
-
         handle:kill(9)
+
         assert.spy(process.kill).was_called(1)
         assert.spy(process.kill).was_called_with(uv_handle, 9)
         assert.spy(kill_handler).was_called(1)
@@ -76,13 +83,14 @@ describe("installer handle", function()
             local kill_handler = spy.new()
             local terminate_handler = spy.new()
             local closed_handler = spy.new()
+
             handle:once("kill", kill_handler)
             handle:once("terminate", terminate_handler)
             handle:once("closed", closed_handler)
             handle.state = "ACTIVE"
             handle.spawn_handles = { { uv_handle = uv_handle2 }, { uv_handle = uv_handle2 } }
-
             handle:terminate()
+
             assert.spy(process.kill).was_called(2)
             assert.spy(process.kill).was_called_with(uv_handle1, 15)
             assert.spy(process.kill).was_called_with(uv_handle2, 15)
diff --git a/vim.yml b/vim.yml
index c5c582b6..72b86e7c 100644
--- a/vim.yml
+++ b/vim.yml
@@ -23,7 +23,9 @@ globals:
             - type: table
               required: false
     mockx.throws:
-        args: []
+        args:
+            - type: any
+              required: false
     mockx.just_runs:
         args: []
     mockx.returns: