fix #11309 in WebSockets by throwing when the underlying connection is closed

[analogrelay]

/cc @stephentoub @Tratcher @moozzyk

Fixes #11309

The if clause is also no longer needed because the only way it could be triggered is if EnsureBufferContainsAsync completed successfully but hadn't read enough data (which only occurs when throwOnPrematureClosure is true).

[stephentoub]

LGTM. Thanks, @anurse.

[analogrelay]

FYI, I do have a test for this in the aspnet copy (aspnet/WebSockets#114) but I don't think there's any infrastructure in this repo for testing actual socket-to-socket communications.

Not being 100% familiar with the merge policies in this repo, can I merge when the builds go green or do we need additional reviewers? Alternatively, I'm happy to let someone else handle the merge :P

[davidsh]

Networking libraries (System.Net.*) are owned by the networking team.

You must add @CIPop @davidsh and @stephentoub to all networking PRs. We must sign off before any merge regardless of green.

We are also changing the testing requirements. We are moving a lot of tests to outerloop (see #11341). You must run outerloop as well.

[stephentoub]

I don't think there's any infrastructure in this repo for testing actual socket-to-socket communications

There isn't... but how hard would that be to add? To use kestrel + websockets, are there NuGet packages we could pull into tests in order to host a local in-proc websocket server for a test? Alternatively, I'd been considering just writing a basic loopback websocket server that established the connection and then handed it off to the same ManagedWebSocket implementation. Hasn't been high on my priority list, though.

[stephentoub]

@davidsh, the change LGTM, all tests passed, and your change hasn't gone in to move tests to outerloop... this can be merged, right?

[stephentoub]

@anurse, just to confirm, you've verified this still passes 100% of the autobahn tests, right?

[CIPop]

I don't think there's any infrastructure in this repo for testing actual socket-to-socket communications.

@anurse @stephentoub @davidsh There actually is. Please try out https://github.com/dotnet/corefx/tree/master/src/Common/tests/System/Net/Prerequisites

[stephentoub]

There actually is

Isn't that about standing up your own servers locally? I was referring to doing so in the tests in CI, such that I could write a test that spawned it's own websocket server, as we do for sockets and http and whatnot with loopback servers.

[analogrelay]

@stephentoub Yes, this passes all Autobahn tests as well as specific unit tests in our repo, as shown by green status on aspnet/WebSockets#114 (comment)

[analogrelay]

You must add @CIPop @davidsh and @stephentoub to all networking PRs. We must sign off before any merge regardless of green.

We are also changing the testing requirements. We are moving a lot of tests to outerloop (see #11341). You must run outerloop as well.

Good to know. It would be helpful if there was a breakdown somewhere in the Contributor's guide as to who owns what parts :). I'll investigate running the outerloop tests. Is that something done manually or triggered as part of the PR checks?

[CIPop]

Isn't that about standing up your own servers locally?

It does at this moment. We need to figure out how to get these activated in at least Outerloop testing (if not CI to replace the #11341 workaround).