search

BEagle1984 / silverback

Silverback is a simple but feature-rich message bus for .NET core (it currently supports Kafka, RabbitMQ and MQTT).
https://silverback-messaging.net
MIT License
259 stars 38 forks source link

fix: Handle exceptions in OutboxQueueHealthCheck (#219) #220

Closed MarkGriffiths95 closed 10 months ago

MarkGriffiths95 commented 10 months ago

Handle exceptions in OutboxQueueHealthCheck and return unhealthy result if exception is caught

MarkGriffiths95 commented 10 months ago

I see the pipeline fails due to rules around throwing exceptions. This is exactly what we're trying to prevent, the unhandled exception results in a 500 error from the server when requesting the health of the application.

Whereas when handling the exception and formatting the response we're able to see all the health checks registered in the application and their status.

BEagle1984 commented 10 months ago

Thank you for the PR. ❤️

The pipeline fails in the build step, because 2 warnings are being issued by the compiler:

/home/vsts/work/1/s/src/Silverback.Integration.HealthChecks/Messaging/HealthChecks/OutboxQueueHealthCheck.cs(62,13): error CA1031: Modify 'CheckHealthAsync' to catch a more specific allowed exception type, or rethrow the exception [/home/vsts/work/1/s/src/Silverback.Integration.HealthChecks/Silverback.Integration.HealthChecks.csproj]
/home/vsts/work/1/s/src/Silverback.Integration.HealthChecks/Messaging/HealthChecks/OutboxQueueHealthCheck.cs(64,46): error CA1062: In externally visible method 'Task<HealthCheckResult> OutboxQueueHealthCheck.CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default(CancellationToken))', validate parameter 'context' is non-null before using it. If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument. [/home/vsts/work/1/s/src/Silverback.Integration.HealthChecks/Silverback.Integration.HealthChecks.csproj]

CA1031 can be suppressed (via attribute please), since we really want to catch all exceptions. CA1062 is just reported because you moved the Check.NotNull inside the try block, but you use the context value in the catch too. Just move the Check.NotNull outside and it will not complain anymore. It's something that will not happen at runtime anyway.

Could you please also get rid of the merge commit and rebase instead? (Not a big deal, I can fix that afterward if needed.)