dotnet / MQTTnet

MQTTnet is a high performance .NET library for MQTT based communication. It provides a MQTT client and a MQTT server (broker). The implementation is based on the documentation from http://mqtt.org/.
MIT License
4.5k stars 1.07k forks source link

WIP: Fix ignored exceptions in MqttHostedServer startup #2102

Open beppemarazzi opened 2 weeks ago

beppemarazzi commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

@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 https://github.com/dotnet/MQTTnet/commit/20eda969213f2ca1ffcafed8657974b97d3be325#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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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...