ASP.NET Core 3.1 hosted service may fail silently

manuelspezzani commented 3 years ago

Describe the bug

The MqttHostedServer hosted service is silently "swallowing" exceptions happening on startup. From the hosting ASP.NET application there is no way to figure out if the MQTT server is running properly or not. Also, no error message is logged.

Which project is your bug related to?

Not sure how to categorize this: the issue is IMO caused by the ASP.NET Core integration layer, in particular by the MqttHostedServer hosted service.

To Reproduce

Step 1 Create a new ASP.NET Core 3.1 web application and register an hosted MQTT server as described here: https://github.com/chkr1011/MQTTnet/wiki/Server#aspnet-core-31-since-mqtt-version-309

I'm currently using v 3.0.16.

Step 2 Make sure port 1883 is in use by starting another MQTT server, or by creating a simple console application binding a socket to port 1883. This is purely to make sure the hosted MQTT server will fail on startup, to reproduce the bug. Of course this may not be the only cause of failure.

Step 3 Start the application created at step 1. Since the port is in use by another process, of course the hosted MQTT server will fail to bind it. But the hosted service (MqttHostedServer) swallows the exception and so there is no way to know that the MQTT server is actually in a "broken" state. Moreover, no error is logged to the "standard" Microsoft logger.

Expected behavior

The issue in my opinion is here: https://github.com/chkr1011/MQTTnet/blob/8f1d4e3c22f3226570eec681bccef3a88c4ebd16/Source/MQTTnet.AspnetCore/MqttHostedServer.cs#L24

        public Task StartAsync(CancellationToken cancellationToken)
        {
            _ = StartAsync(_options);
            return Task.CompletedTask;
        }

That method should return the Task from the internal StartAsync method, i.e.:

        public Task StartAsync(CancellationToken cancellationToken)
        {
            return StartAsync(_options);
        }

That is for sure a breaking change because in case of error the entire web application will fail to start but, in my opinion, it's the right thing to do. As an alternative, I guess we need a way to figure out if the MqttServer is actually running or not. That way, for example, it will be possible to create an ASP.NET Core Health Check (https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/health-checks?view=aspnetcore-3.1) to monitor the status of the hosted service.

manuelspezzani commented 3 years ago

That is for sure a breaking change because in case of error the entire web application will fail to start but, in my opinion, it's the right thing to do.

Guys, do you have an opinion on this? Are there any reasons why the hosted service is swallowing startup exceptions? I'd be more than happy to provide a pull request, but I don't want to break existing scenarios...

chkr1011 commented 3 years ago

@JanEggers Do you have any advise here?

JanEggers commented 3 years ago

im not sure why I did it this way. I think it was a deadlock issue with some callback code in startup. I personally would also prefere that if the server does not work the whole app get teared down as its useless otherwise.

in regards to health checks: i would use a mqtt client to verify the service is up and not fiddle with the hostedservice or other single classes. with a client you can verify the availability on protocol level.

dotnet / MQTTnet