Skip to content
This repository was archived by the owner on Aug 18, 2021. It is now read-only.
This repository was archived by the owner on Aug 18, 2021. It is now read-only.

LightWorker won't properly handle asynchronous methods #23

Open
@guilhermeluizsp

Description

@guilhermeluizsp

The method InvokeProcess inside the LightWorker class is responsible for invoking the target method (decorated with the [Topic] attribute) when a new message arrives (from a service bus, for example).

https://github.com/Avanade/Liquid-Application-Framework/blob/d625dff6d3af90fc9aba210f8898564b7b4f02c7/src/Liquid.Activation/Worker/LightWorker.cs#L130-L174

This method has a couple of problems:

It doesn't await asynchronous methods

When a worker method is asynchronous, it is never awaited. This will lead to unobserved exceptions when an exception is thrown inside the method.

Depending on how the system is set up, this may cause the whole process to be terminated (crashed). This is the default behavior for .NET 4 and below. Fortunately, Microsoft changed it starting from .NET 4.5 and above (including .NET Core). Note that it is still possible to configure the application to keep the old behavior.

It doesn't catch deferred exceptions

We can see at lines 166, 167, 168 and 169, the code checks if the returned Task is in a faulted state (i.e an exception was thrown from within it). If it is, the exception is re-thrown.

https://github.com/Avanade/Liquid-Application-Framework/blob/d625dff6d3af90fc9aba210f8898564b7b4f02c7/src/Liquid.Activation/Worker/LightWorker.cs#L166-L170

As mentioned before, the task is not awaited, so this block of code will not be able to catch deferred exceptions. Consider these two methods:

public async Task EarlyException()
{
    throw new Exception();
    await Task.Delay(100);
}

public async Task DeferredException()
{
    await Task.Delay(100);
    throw new Exception();
}

We know that, when executing asynchronous methods, it will execute synchronously until it hits the await keyword (and the following awaitable is not completed yet). If these methods were worker methods (methods decorated with the [Topic] attribute), that block of code would only catch the first method (EarlyException), as the exception is immediately thrown. The second method would eventually throw an unobserved exception.

It doesn't re-throw exceptions correctly

As we can see, at the line 169, the exception is re-thrown if the task is faulted.

https://github.com/Avanade/Liquid-Application-Framework/blob/d625dff6d3af90fc9aba210f8898564b7b4f02c7/src/Liquid.Activation/Worker/LightWorker.cs#L169

However, the Exception property holds an AggregateException, which wraps the real exception. We shouldn't re-throw this AggregateException, but its inner exception.

We can't re-throw it directly (using throw result.Exception.InnerException because it would lose its original Stack Trace, so we must use the ExceptionDispatchInfo class, which will correctly re-throw the exception.

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingcomplexity:lowThe complexity of the issue is lowpriorityThis issue should be acted upon before other issues.size:smallThe size of the issue is small

Type

No type

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions