Don't call close if connect does not succeed

[BrennanConroy]

Fixes #14075

Description

There is a race when falling back from WebSockets to other transports that will prevent fallback from working. This code was refactored in 3.0 which caused this regression. When a WebSocket connection failed to start, the WebSocket transport code would signal both that the connection failed to start and that the connection was closed. This would cause a race in the SignalR logic and if the wrong path wins the race, the connection is closed entirely instead of falling back to other transports.

Customer Impact

In situations where an intermediary (i.e. not the client, and not the app server) doesn't support WebSockets, the client may fail to connect where previously it would have fallen back to a different transport. Generally this wouldn't have a huge impact except that by default Azure App Service has an intermediary that doesn't support WebSockets. It can be enabled by users, but many customers don't enable it or don't wish to enable it. It is also a regression of a core functionality of SignalR (fallback) in 3.0.

Regression?

Yes, regression in 3.0 from the 2.2 behavior

Risk

Low, added test coverage to prevent this kind of issue in the future

[halter73]

Nit: might be nice to test this with an ErrorEvent.

[halter73]

Why the [Do Not Merge]?

[BrennanConroy]

Why the [Do Not Merge]?

To not scare Doug 😛

Also, because 3.0 isn't ready for patch work.

[analogrelay]

@BrennanConroy put the servicing goop in (Customer Impact, a brief description, risk, etc.)

[BrennanConroy]

put the servicing goop in (Customer Impact, a brief description, risk, etc.)

@anurse Done, if you'd like to wordsmith it a bit (since you're better at that than I am) go for it.

[dougbu]

🥇

[analogrelay]

Approved by Tactics for 3.0 servicing. Please ⚠️ do not merge ⚠️ for right now though. I think we need to do branding updates before merging servicing fixes (cc @aspnet/build @Pilchie)

[dougbu]

Yeah, I'm working on those branding updates -- an expansion of aspnet/AspNetCore-Internal#3153 that I started on last night

[analogrelay]

Branches are ✔ Open ✔ for 3.0.1 changes. Since this is approved and has the patch config goop. This can be merged when ready (and builds are green).

[BrennanConroy]

Note: People who can merge, don't do so without talking to me first please. Need to verify the runtime is updated etc.

[BrennanConroy]

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=372077&view=logs

[aspnet-hello]

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/3156
https://github.com/aspnet/AspNetCore-Internal/issues/2617
https://github.com/aspnet/AspNetCore-Internal/issues/2408
https://github.com/aspnet/AspNetCore-Internal/issues/3184
https://github.com/aspnet/AspNetCore-Internal/issues/2673

[DNF-SaS]

Guys, whatsup with this issue - it's really hitting us hard because we have firewalls without WebSocket-Support!

[BrennanConroy]

If you know that WebSockets are not supported you can set the supported transports on your hub endpoint so clients don't even try to use WebSockets

See the Transports option in https://docs.microsoft.com/en-us/aspnet/core/signalr/configuration?view=aspnetcore-3.0&tabs=dotnet#advanced-http-configuration-options

[analogrelay]

@DNF-SaS It'll be fixed in the upcoming 3.0.1 patch. The date is TBD as there are some blocking issues we are working to resolve but we are working on getting out as quickly as possible. It'll also be fixed in the 3.1 release scheduled by the end of the year. I know this is a bad one, we're working on getting a fix out ASAP.

My recommendation for now would be to hold the update to 3.0 if you're affected by this. Watch the .NET Blog for a post when the 3.0.1 patch is available.

[paralaxsd]

If you know that WebSockets are not supported you can set the supported transports on your hub endpoint so clients don't even try to use WebSockets

See the Transports option in https://docs.microsoft.com/en-us/aspnet/core/signalr/configuration?view=aspnetcore-3.0&tabs=dotnet#advanced-http-configuration-options

It wasn't immediately clear to me where to set this up exactly. Should anyone else struggle in the time until 3.0.1, this worked for me as temp workaround in Startup.Configure:

app.UseEndpoints(endpoints =>
{
    endpoints.MapBlazorHub(options =>
    {
        options.Transports = HttpTransportType.LongPolling;
    });
    endpoints.MapFallbackToPage("/_Host");
});