Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Fix ignored exceptions in MqttHostedServer startup #2102

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

beppemarazzi
Copy link

The exceptions raised during the startup of MqttHostedServer are completely ignored.
Nothing on the logs, the application continue to run, and the MqttServer.StartedAsync event is never raised leaving the application in an inconsistent state...
I found this issue because in my application I'm also using the Tcp endpoint (on the default port 1883), but if the port is already in use obviously the Tcp listener fails with an exception.

This PR fixes the issue shutting down the application in this case.
If the user has a service registration for Microsoft.Extensions.Logging.ILogger (quite common in real life scenario) an error is also logged.

@xljiulang
Copy link
Contributor

I think the correct implementation should be: await the MqttServer.StartAsync() behavior in BackgroundService so that the exception can be propagated. The specific implementation is similar to https://github.com/xljiulang/MQTTnet/blob/master/Source/MQTTnet.AspnetCore/Internal/AspNetCoreMqttHostedServer.cs#L39

@beppemarazzi
Copy link
Author

@xljiulang

I think the correct implementation should be: await the MqttServer.StartAsync() behavior in BackgroundService so that the exception can be propagated.

This was also my first thought.
And it was exactly the earlier (up to 2020) implementation (see https://github.com/dotnet/MQTTnet/blob/66048931a2bae650582e638b010cee0104a797f8/Source/MQTTnet.AspnetCore/MqttHostedServer.cs) but with 20eda96#diff-7489ffb431847a1474c7a8fa6dcd02fb1ed29ec54a204f6637789014ed277fb6R21-R27 the implementation was changed (i don't know the reason. Probably @JanEggers knows?...)
And moving forward with this one https://github.com/dotnet/MQTTnet/blob/e18a91a4b59bc312aa9acf9be575401d07b793e4/Source/MQTTnet.AspnetCore/MqttHostedServer.cs the implementation was changed again dispatching the MqttServer.StartAsync() after the complete stratup of the Host is notified by IHostApplicationLifetime callback. See #2025 for details.

If i understand correctly this is a sort of chicken-and-egg problem: the MqttServer must start after the host because of #2025 BUT if we want to rely on the failure logic at host startup in case of exception during the MqttServer initialization we cannot wait...

@xljiulang
Copy link
Contributor

If you start MqttnetServer in the IHostedService.StartAsync method, there will generally be no problems. The worst case is that you unfortunately use MqttTcpServerAdapter, which increases the service startup time. If it is used as a windows service process, it may time out and fail.

@xljiulang
Copy link
Contributor

Currently, it is changed to delayed startup. The logic of the existing code is vulnerable: when Asp.netcore is successfully started and MqttServer is not started, if there is mqtt connecting through asp.netcore, MqttConnectionHandler will be triggered to call OnConnectedAsync instead of StartAsync.

@xljiulang
Copy link
Contributor

If you want to delay starting MqttnetServer, you should fix MqttConnectionHandleras follows:

    public override async Task OnConnectedAsync(ConnectionContext connection)
    {
        var clientHandler = ClientHandler;
        if (clientHandler == null)
        {
            connection.Abort();
            _logger.Publish(MqttNetLogLevel.Warning, nameof(MqttConnectionHandler), $"{nameof(MqttServer)} has not been started yet.", null, null);
            return;
        }
        ...
}

@beppemarazzi beppemarazzi changed the title Fix ignored exceptions in MqttHostedServer startup WIP: Fix ignored exceptions in MqttHostedServer startup Nov 14, 2024
@beppemarazzi
Copy link
Author

beppemarazzi commented Nov 14, 2024

@xljiulang
Could you take a look at my last commit?
I just realizer it's IMHO the most obvious neat, clean KISS solution: relying on standard built-in BackgroundService logic!!!

EDIT: sorry, I'm late!!! 😄 I've just took a look at your #2103 and you probably came to the same conclusion...

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