[browser][wasm] Wasm websockets support by kjpou1 · Pull Request #37962 · dotnet/runtime

kjpou1 · 2020-06-16T11:11:12Z

No description provided.

ghost · 2020-06-16T11:11:17Z

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

.../System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ClientWebSocket.cs

…websockets

…ClientWebSocket creation.

....Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ClientWebSocketOptions.cs

marek-safar · 2020-06-18T18:48:36Z

...s/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ReceivePayload.cs

+
+namespace System.Net.WebSockets
+{
+    internal sealed class ReceivePayload


Where is this type used?

Used by BrowserWebSocket.cs to handle the received message queue

It could be readonly struct

src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/ClientWebSocket.cs

src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs

- change accessor of _requestedSubProtocols to private - condition more code in WebSocketHandle.Managed.cs to not access _requestedSubProtocols

…websockets # Conflicts: # src/libraries/System.Net.WebSockets.Client/src/System.Net.WebSockets.Client.csproj # src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs

kjpou1 · 2020-06-24T05:52:02Z

@marek-safar @stephentoub ping for a review when available.

...s/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ReceivePayload.cs

marek-safar · 2020-06-24T09:39:40Z

...s/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ReceivePayload.cs

+
+namespace System.Net.WebSockets
+{
+    internal sealed class ReceivePayload


It could be readonly struct

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

stephentoub · 2020-06-24T12:36:05Z

....Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ClientWebSocketOptions.cs

@@ -0,0 +1,117 @@
+// Licensed to the .NET Foundation under one or more agreements.


Do we need to specialize this file for the browser? Or instead could ClientWebSocket.ConnectAsync just throw PNSE if it finds options set that aren't supported?

The list of dependencies from the desktop version is much bigger (size savings) but it'd be nice to extract the common code to the shared file

The list of dependencies from the desktop version is much bigger (size savings)

Only if the consumer uses them, though, right? Otherwise we should be able to ensure the relevant stuff is linked away?

Yes if we don't call them internally, for example, to check if there were set.

...s/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ReceivePayload.cs

src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

…ockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: campersau <buchholz.bastian@googlemail.com>

src/libraries/System.Net.WebSockets.Client/src/Resources/Strings.resx

src/libraries/System.Net.WebSockets.Client/src/System.Net.WebSockets.Client.csproj

…ockets.Client.csproj Co-authored-by: Maxim Lipnin <mlipnin@gmail.com>

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

marek-safar · 2020-07-01T14:56:29Z

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

+
+            WebSocketValidate.ValidateArraySegment(buffer, nameof(buffer));
+
+            _writeBuffer ??= new MemoryStream();


Is this intentional? Caling SendAsync repeatably will grow MemoryStream with no limit

The write buffer is nulled out below so it will be recreated.

MemoryStream writtenBuffer = _writeBuffer; _writeBuffer = null;

then why do you need ??=

because the message needs to be buffered until end of message is reached. Once end of message is reached it gets nulled out and needs to be recreated.

…websockets

- These are now released properly after reference counting went in.

marek-safar

LGTM from me. @stephentoub could you do another pass

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

stephentoub · 2020-07-02T14:02:59Z

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

+            _writeBuffer.Write(buffer.Array!, buffer.Offset, buffer.Count);
+
+            if (!endOfMessage)
+                return Task.CompletedTask;


The browser websocket API isn't capable of sending messages split over multiple fragments? The endOfMessage argument elsewhere translates into the FIN bit in the websocket frame.

We do not have access to the websocket frame through the browser api. Also, from my understanding there is not the concept of EndOfMessage. Not that I can find anyway.

https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/send expects the full message so we need to buffer somewhere. An alternative to the MemoryStream buffer would be constructing a chain of Blobs on the JS side and calling send on that when endOfMessage is true.

It's fine for the buffering to be in managed. I was questioning the buffering all-up, but if the websocket API doesn't support fragments, then ok.

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

…ockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

kjpou1 added 2 commits June 16, 2020 09:40

[browser][wasm] Initial addition of WebSockets support

08c9235

Clean up usings

9369e51

Dotnet-GitSync-Bot added the area-System.Net label Jun 16, 2020

kjpou1 requested review from lewing, marek-safar and vargaz June 16, 2020 11:11

stephentoub reviewed Jun 16, 2020

View reviewed changes

.../System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/ClientWebSocket.cs Outdated Show resolved Hide resolved

kjpou1 added 4 commits June 17, 2020 07:21

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

c336c18

…websockets

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

c8942f6

…websockets

Create and use a WebSocketHandle.Browser.cs module

139fd62

Add conditional TARGETS_BROWSER so as not to throw Proxy PNSE during …

04ffbbe

…ClientWebSocket creation.

jaredpar mentioned this pull request Jun 17, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

kjpou1 added 2 commits June 18, 2020 15:33

Update WebSockets code

27dc6d3

Removing debug line

6fa2691

kjpou1 marked this pull request as ready for review June 18, 2020 14:43

marek-safar reviewed Jun 18, 2020

View reviewed changes

kjpou1 added 5 commits June 19, 2020 07:31

Remove ThrowIfReadOnly method to address review comments

cb5f322

Address review comments

cb46e3e

- change accessor of _requestedSubProtocols to private - condition more code in WebSocketHandle.Managed.cs to not access _requestedSubProtocols

Address review comments

192934f

Address review comments

cb21d7d

Change to be PlatformNotSupportedException

d32754c

kjpou1 requested review from marek-safar and stephentoub June 19, 2020 10:49

lewing added this to the 5.0.0 milestone Jun 22, 2020

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

3b4e5ea

…websockets # Conflicts: # src/libraries/System.Net.WebSockets.Client/src/System.Net.WebSockets.Client.csproj # src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/WebSocketHandle.Managed.cs

marek-safar reviewed Jun 24, 2020

View reviewed changes

stephentoub reviewed Jun 24, 2020

View reviewed changes

stephentoub requested changes Jun 24, 2020

View reviewed changes

campersau reviewed Jul 1, 2020

View reviewed changes

Address TaskCompletionSource on connect review comments

df2fb14

campersau reviewed Jul 1, 2020

View reviewed changes

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs Outdated Show resolved Hide resolved

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs Outdated Show resolved Hide resolved

kjpou1 and others added 6 commits July 1, 2020 11:11

Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebS…

30daa1d

…ockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: campersau <buchholz.bastian@googlemail.com>

Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebS…

67ef8d8

…ockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: campersau <buchholz.bastian@googlemail.com>

Fix Connect not completing task on error.

417ab90

Cleanup reference

b993af5

Remove redundant error check of memorystream buffer.

eb81a12

cleanup

9f84ea9

MaximLipnin reviewed Jul 1, 2020

View reviewed changes

kjpou1 and others added 5 commits July 1, 2020 15:31

Update src/libraries/System.Net.WebSockets.Client/src/System.Net.WebS…

02bbabe

…ockets.Client.csproj Co-authored-by: Maxim Lipnin <mlipnin@gmail.com>

Update src/libraries/System.Net.WebSockets.Client/src/System.Net.WebS…

28f750b

…ockets.Client.csproj Co-authored-by: Maxim Lipnin <mlipnin@gmail.com>

Update src/libraries/System.Net.WebSockets.Client/src/System.Net.WebS…

f68cb63

…ockets.Client.csproj Co-authored-by: Maxim Lipnin <mlipnin@gmail.com>

Update src/libraries/System.Net.WebSockets.Client/src/System.Net.WebS…

89dccc2

…ockets.Client.csproj Co-authored-by: Maxim Lipnin <mlipnin@gmail.com>

Fix typo

f42877a

marek-safar reviewed Jul 1, 2020

View reviewed changes

kjpou1 added 5 commits July 1, 2020 17:13

clean up buffer code

7a73ccb

Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…

ead512c

…websockets

Modify callbacks to use lambda function.

60f4d87

- These are now released properly after reference counting went in.

Fix object leak of delegate and clean up deprecated code.

c73b9d9

extract lambda into method

c448c07

marek-safar approved these changes Jul 2, 2020

View reviewed changes

stephentoub approved these changes Jul 2, 2020

View reviewed changes

kjpou1 and others added 3 commits July 2, 2020 16:45

Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebS…

05e5dc6

…ockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>

Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebS…

b1edef3

…ockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>

Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebS…

63becae

…ockets/BrowserWebSockets/BrowserWebSocket.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>

campersau reviewed Jul 2, 2020

View reviewed changes

...System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs Outdated Show resolved Hide resolved

Review comments addressed

dc7f224

lewing merged commit 9715a9c into dotnet:master Jul 2, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

		@@ -0,0 +1,117 @@
		// Licensed to the .NET Foundation under one or more agreements.


		WebSocketValidate.ValidateArraySegment(buffer, nameof(buffer));

		_writeBuffer ??= new MemoryStream();

Conversation

kjpou1 commented Jun 16, 2020

Uh oh!

ghost commented Jun 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kjpou1 commented Jun 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjpou1 Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marek-safar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjpou1 Jul 1, 2020 •

edited

Loading