Skip to content

Fix exception handling for multiple Task-based event handlers#603

Merged
ikkentim merged 2 commits into
ikkentim:mainfrom
DevD4v3:patch-1
Jun 30, 2026
Merged

Fix exception handling for multiple Task-based event handlers#603
ikkentim merged 2 commits into
ikkentim:mainfrom
DevD4v3:patch-1

Conversation

@DevD4v3

@DevD4v3 DevD4v3 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes an issue where exceptions from Task-based event handlers could go unobserved when multiple handlers were registered for the same event.

Problem

EventDispatcher only calls HandleTask on the final event result returned by InnerInvoke.

As a result, when multiple event handlers return a Task, only the last task is observed for exceptions. Any faulted tasks returned by previous handlers are silently ignored.

For example:

public class SystemA : ISystem
{
    [Event]
    public async Task OnPlayerConnect(Player player)
    {
        throw new Exception("A");
    }
}

public class SystemB : ISystem
{
    [Event]
    public async Task OnPlayerConnect(Player player)
    {
        await Task.Delay(100);
        Console.WriteLine("B");
    }
}

Current behavior:

(no exception logged)

Expected behavior:

[ERR] Unhandled exception during: async-event
System.Exception: A

Root Cause

InnerInvoke iterates through all registered handlers and stores only the last non-null result:

foreach (var targetSite in @event.TargetSites)
{
    var targetResult = targetSite.Invoke(...);

    result = targetResult;
}

Later, HandleTask is only called for the final result:

var result = invoke(arguments);

if (result is Task task)
{
    HandleTask(task);
}

This means that faulted tasks returned by earlier handlers are never observed.

Solution

Observe tasks as soon as they are returned by an event handler:

if (targetResult is Task task)
{
    HandleTask(task);
}

This ensures that every task returned by every event handler is monitored for exceptions, regardless of the final event result.

Validation

The issue was reproduced using multiple Task-based handlers registered for the same event:

public class TestSystemA : ISystem
{
    [Event]
    public async Task OnPlayerConnect(Player player)
    {
        await Task.Delay(100);
        throw new Exception("A");
    }
}

public class TestSystemB : ISystem
{
    [Event]
    public async Task OnPlayerConnect(Player player)
    {
        await Task.Delay(200);
        throw new Exception("B");
    }
}

public class TestSystemC : ISystem
{
    [Event]
    public async Task OnPlayerConnect(Player player)
    {
        await Task.Delay(300);
        throw new Exception("C");
    }
}

After this change all exceptions are correctly reported:

[ERR] Unhandled exception during: async-event
System.Exception: A

[ERR] Unhandled exception during: async-event
System.Exception: B

[ERR] Unhandled exception during: async-event
System.Exception: C

Comment thread src/SampSharp.OpenMp.Entities/Events/EventDispatcher.cs Outdated
Comment thread src/SampSharp.OpenMp.Entities/Events/EventDispatcher.cs Outdated
@DevD4v3 DevD4v3 requested a review from ikkentim June 19, 2026 18:49
@DevD4v3

DevD4v3 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@ikkentim The requested changes have been implemented.
When you have a moment, could you review the PR again?

@DevD4v3

DevD4v3 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

@ikkentim This PR is now ready to be merged.

@ikkentim ikkentim merged commit 6f07302 into ikkentim:main Jun 30, 2026
1 check passed
@DevD4v3 DevD4v3 deleted the patch-1 branch June 30, 2026 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants