[ASPNET Core 2.1] src\SignalR\clients\csharp\Http.Connections.Client\src\HttpConnection.cs should null check logging scope before disposing

[arianmotamedi]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

HttpConnection.cs in SignalR client (aspnet core 2.1) is disposing its ILogger.Scope object without doing a null check. LoggerExtensions.BeginScope<>() documentation says this method may return null, so it needs to be properly null checked before calling .Dispose() on it otherwise it throws a null ref exception at runtime.

Expected Behavior

_scopeDisposable should be null-checked before disposing.

Steps To Reproduce

For whatever reason, the scope isn't null in Microsoft.Extensions.Logging 2.2 even though the docs say this version could still return null. We can see this when running Microsoft.AspNetCore.SignalR.Client.Tests.HttpConnectionTests+Negotiate.NegotiateReturnedConenctionIdIsSetOnConnection test - as it's using LogSinkLogger, which returns null in its BeginScope() override.

Upgrading Microsoft.Extensions.Logging references and its dependencies to 3.1 is correctly calling the override and returning null, but that dispose method above throws a null ref exception at runtime.

Exceptions (if any)

No response

.NET Version

4.6.2

Anything else?

ASPNET Core 2.1

[mgravell]

This appears to not be a problem in the current code - fixed here (which landed after 2.1.33 and at the latest 2.2.2 - I can't check 2.2.1 for some reason); I agree that this is technically in error, and the fix is pretty trivial ( _scopeDisposable?.Dispose();) - but: I'm not sure we're still issuing updates to 2.1, so I'm not sure this would be a useful change - I'll check my understanding there.

[arianmotamedi]

Thanks @mgravell. Stephen has the same sentiment on the open PR for the fix: #47471. I didn't realize aspnet core 2.1 is strictly in servicing mode - makes sense to not want to release a patch for this change.