Streaming Interop Followup Items by TanayParikh · Pull Request #33916 · dotnet/aspnetcore

TanayParikh · 2021-06-28T18:58:33Z

Resolves:

Blazor Streaming Interop | JS to DotNet #33491 (comment)
Reduce (max) buffer sizing: Blazor Streaming Interop | JS to DotNet #33491 (comment)
Support ArrayBuffers directly: Blazor Streaming Interop | JS to DotNet #33491 (comment)
- Looked into this some more, it seems ArrayBuffer's aren't normally used directly and we don't have element access, so I don't think we need to directly support them. I've updated the error message in the original PR.
Timeout tests: Blazor Streaming Interop | JS to DotNet #33491 (comment)
- Streaming CI Debugging #33917

Separate PR:

Add WASM/WebView support: Blazor Streaming Interop | JS to DotNet #33491 (comment)
- Blazor WebAssembly & WebView JS to .NET Streaming Interop Support #33986

* CI Debugging * CiData message * CiData message * Update RemoteJSDataStream.cs * Remove Task.Delay * Update RemoteJSDataStream.cs * Update RemoteJSDataStream.cs

TanayParikh · 2021-06-29T05:58:49Z

+            long pauseIncomingBytesThreshold,
+            long resumeIncomingBytesThreshold,


Per David Fowler's suggestion #33491 (comment)

javiercn · 2021-06-29T09:50:08Z

 *REMOVED*static Microsoft.JSInterop.JSRuntimeExtensions.InvokeVoidAsync(this Microsoft.JSInterop.IJSRuntime! jsRuntime, string! identifier, params object![]! args) -> System.Threading.Tasks.ValueTask
 static Microsoft.JSInterop.JSRuntimeExtensions.InvokeVoidAsync(this Microsoft.JSInterop.IJSRuntime! jsRuntime, string! identifier, params object?[]? args) -> System.Threading.Tasks.ValueTask
-virtual Microsoft.JSInterop.JSRuntime.ReadJSDataAsStreamAsync(Microsoft.JSInterop.IJSStreamReference! jsStreamReference, long totalLength, long maxBufferSize, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task<System.IO.Stream!>!
+virtual Microsoft.JSInterop.JSRuntime.ReadJSDataAsStreamAsync(Microsoft.JSInterop.IJSStreamReference! jsStreamReference, long totalLength, long pauseIncomingBytesThreshold, long resumeIncomingBytesThreshold, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task<System.IO.Stream!>!


I'm not a big fan of this API.

It exposes a bunch of internal details and it does so in a way that is problematic and that can make it easy to introduce security issues if you are not careful. The defaults need to be safe (the default can't never be "unlimited" on a server app) and we should add a remark on the comments about the security implications of these parameters.

@javiercn Would you consider it sufficient to:

have some default like 100KB or so

remove the behavior of "zero means unlimited" so that if someone really wants unlimited they have to pass int.MaxValue or similar

include a mention in the XML docs for the parameter saying something like Avoid specifying an excessively large value because this could allow clients to exhaust server memory.

?

That sounds about right.

We would need to figure out a good default. It's a trade-off between speed and availability I think

To clarify this here, we do have a safe default (indicated by -1, zero is not the default). Users should be working with the JSStreamReference and not the JSRuntime methods seen here. This follows the exact pattern seen in the PipeOptions, and we utilize these parameters directly with the Pipe during initialization.

Documentation: https://github.com/dotnet/runtime/blob/9fb6e818cda2e4364e085fe0b8ecebed97db9596/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeOptions.cs#L22-L23

Default Args:
https://github.com/dotnet/runtime/blob/9fb6e818cda2e4364e085fe0b8ecebed97db9596/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeOptions.cs#L30-L31

What the safe defaults translate to:
https://github.com/dotnet/runtime/blob/9fb6e818cda2e4364e085fe0b8ecebed97db9596/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeOptions.cs#L47-L69

(65k and 32.5k bytes). Originally we were using a 100kb but per #33491 (comment), it seemed like using the Pipe defaults was preferred.

Are we okay keeping with this general approach in this case? Action items:

Update the comments to provide more clarity on -1 and security implications of using large sizes

Reflect the safe defaults -1 in the JSRuntime as well.

Would 7785751 resolve these concerns?

javiercn

I have a few questions that I've put on the comments.

In addition to those questions, I would like us to add an API that exposes the PipeReader directly. This essentially saves an intermediate copy from the Stream. Look for other APIs that expose the PipeReader and follow the same pattern.

greenEkatherine · 2021-06-29T11:36:18Z

/azp run

azure-pipelines · 2021-06-29T11:36:36Z

Azure Pipelines successfully started running 2 pipeline(s).

SteveSandersonMS · 2021-06-29T13:33:03Z

Looked into this some more, it seems ArrayBuffer's aren't normally used directly and we don't have element access, so I don't think we need to directly support them. I've updated the error message in the original PR.

I don't really mind, and am fine with requiring people to supply a typed array. We can always add support for more cases in the future if we want.

However for clarity, an arraybuffer is just a store of bytes that can be viewed as being one of several possible types (Uint8Array, Uint32Array, etc.). So if we receive an arraybuffer, it's always correct and valid to read the bytes directly from it using any element size we choose arbitrarily. For example we could freely new Uint8Array(arrayBuffer) to get access to the contents as raw bytes.

BrennanConroy · 2021-06-29T16:41:20Z

+                jsStreamReference,
+                totalLength: 15,
+                maximumIncomingBytes: 10_000,
+                jsInteropDefaultCallTimeout: TimeSpan.FromSeconds(30), // Note we're using a 30 second timeout for this test


Generally we try to make tests as quick as possible, we do this by
a) not using real timers (mock timers so we control when they tick and how much time they think has passed)
b) avoiding timers if possible (presumably this test is actually testing a timeout so b. doesn't apply)
c) using much lower values for timeouts if a. or b. can't be done

Additionally, relying on a real timeout creates potential flakiness because what if the machine is really slow and the test doesn't do the work you expected before the timeout occurs?

Couple examples:

aspnetcore/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2TimeoutTests.cs

Lines 26 to 35 in cee22ed

var mockSystemClock = _serviceContext.MockSystemClock;

var limits = _serviceContext.ServerOptions.Limits;

_timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks);

CreateConnection();

_connectionTask = _connection.ProcessRequestsAsync(new DummyApplication(_noopApplication));

AdvanceClock(limits.KeepAliveTimeout + Heartbeat.Interval);

aspnetcore/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs

Lines 2690 to 2708 in 65276b0

var clock = new MockSystemClock();

var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services =>

services.Configure<HubOptions>(options =>

options.KeepAliveInterval = TimeSpan.FromMilliseconds(interval)), LoggerFactory);

var connectionHandler = serviceProvider.GetService<HubConnectionHandler<MethodHub>>();

connectionHandler.SystemClock = clock;

using (var client = new TestClient(new NewtonsoftJsonHubProtocol()))

{

var connectionHandlerTask = await client.ConnectAsync(connectionHandler);

await client.Connected.DefaultTimeout();

// Trigger multiple keep alives

var heartbeatCount = 5;

for (var i = 0; i < heartbeatCount; i++)

{

clock.UtcNow = clock.UtcNow.AddMilliseconds(interval + 1);

client.TickHeartbeat();

}

Last thing, when using await you should consider putting .DefaultTimeout() on the calls so that any hangs will fail the test (and tell you where they hung) and not timeout the test run instead.
Example:

aspnetcore/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.cs

Lines 4453 to 4461 in 65276b0

await client.Connected.OrThrowIfOtherFails(connectionHandlerTask).DefaultTimeout();

await client.BeginUploadStreamAsync("invocation", nameof(MethodHub.UploadDoesWorkOnComplete), streamIds: new[] { "id" }, args: Array.Empty<object>()).DefaultTimeout();

await client.SendHubMessageAsync(new StreamItemMessage("id", "hello")).DefaultTimeout();

await client.DisposeAsync().DefaultTimeout();

await connectionHandlerTask.DefaultTimeout();

Sorry if this ended up looking like a bit of a rant, but I really want our tests to be reliable.

Thanks @BrennanConroy appreciate the feedback. I went the route of using a TaskCompletionSource to match the pattern used in other src/Components/tests tests to keep consistency. I also significantly reduced the timeout values to avoid long running tests, and the TaskCompletionSource approach should eliminate flakiness as we'll be able to await the timeout. I've also updated the tests to leverage DefaultTimeout().

1cd162f

TanayParikh · 2021-06-29T17:42:47Z

In addition to those questions, I would like us to add an API that exposes the PipeReader directly. This essentially saves an intermediate copy from the Stream. Look for other APIs that expose the PipeReader and follow the same pattern.

Will expose a public PipeReader property like we do here or here.

Edit: Done in 94ea801

TanayParikh · 2021-06-29T20:26:16Z

So if we receive an arraybuffer, it's always correct and valid to read the bytes directly from it using any element size we choose arbitrarily. For example we could freely new Uint8Array(arrayBuffer) to get access to the contents as raw bytes.

I'll update in #33900 as that area is already being reworked in #33900.

Edit: Done here.

TanayParikh · 2021-06-30T02:31:35Z

/azp run

azure-pipelines · 2021-06-30T02:31:53Z

Azure Pipelines successfully started running 2 pipeline(s).

TanayParikh · 2021-07-08T19:40:31Z

@javiercn @SteveSandersonMS could I please get a review, hoping to get it in for Preview 7.

SteveSandersonMS

Nice - this looks good to me! Might be good to ask @javiercn if he is happy with all the resolutions here, but I know you've been waiting a while to merge this and we're getting close to the cutoff date, so it would probably be fine to merge and let @javiercn comment later if he needs :)

Streaming Interop Followup Items

3836301

TanayParikh added the area-blazor Includes: Blazor, Razor Components label Jun 28, 2021

Streaming CI Debugging (#33917)

1ea5be4

* CI Debugging * CiData message * CiData message * Update RemoteJSDataStream.cs * Remove Task.Delay * Update RemoteJSDataStream.cs * Update RemoteJSDataStream.cs

TanayParikh marked this pull request as ready for review June 29, 2021 05:57

TanayParikh requested review from a team and Pilchie as code owners June 29, 2021 05:57

TanayParikh requested review from SteveSandersonMS and javiercn June 29, 2021 05:57

TanayParikh commented Jun 29, 2021

View reviewed changes

javiercn reviewed Jun 29, 2021

View reviewed changes

Comment thread src/Components/Server/test/Circuits/RemoteJSDataStreamTest.cs Outdated

javiercn reviewed Jun 29, 2021

View reviewed changes

Comment thread src/Components/Server/test/Circuits/RemoteJSDataStreamTest.cs Outdated

javiercn reviewed Jun 29, 2021

View reviewed changes

Comment thread src/JSInterop/Microsoft.JSInterop/src/IJSStreamReference.cs Outdated

javiercn reviewed Jun 29, 2021

View reviewed changes

SteveSandersonMS reviewed Jun 29, 2021

View reviewed changes

Comment thread src/Components/Server/test/Circuits/RemoteJSDataStreamTest.cs Outdated

SteveSandersonMS reviewed Jun 29, 2021

View reviewed changes

Comment thread src/JSInterop/Microsoft.JSInterop/src/IJSStreamReference.cs

BrennanConroy reviewed Jun 29, 2021

View reviewed changes

TanayParikh added 4 commits June 29, 2021 11:30

Merge branch 'main' into taparik/streamingFollowups

1461ee4

PR Feedback (Including PipeReader Property)

94ea801

Improve documentation on safe defaults

7785751

Task Completion Source & DefaultTimeout

1cd162f

TanayParikh requested review from SteveSandersonMS and javiercn June 29, 2021 20:24

InvalidateLastDataReceivedTimeForTimeout

7961c34

TanayParikh requested review from a team and MackinnonBuck July 2, 2021 00:04

TanayParikh mentioned this pull request Jul 7, 2021

Blazor Support Large File Uploads #33638

Closed

SteveSandersonMS approved these changes Jul 9, 2021

View reviewed changes

TanayParikh merged commit 774b2af into main Jul 9, 2021

TanayParikh deleted the taparik/streamingFollowups branch July 9, 2021 17:19

ghost added this to the 6.0-preview7 milestone Jul 9, 2021

		long pauseIncomingBytesThreshold,
		long resumeIncomingBytesThreshold,

	var mockSystemClock = _serviceContext.MockSystemClock;
	var limits = _serviceContext.ServerOptions.Limits;

	_timeoutControl.Initialize(mockSystemClock.UtcNow.Ticks);

	CreateConnection();

	_connectionTask = _connection.ProcessRequestsAsync(new DummyApplication(_noopApplication));

	AdvanceClock(limits.KeepAliveTimeout + Heartbeat.Interval);

	var clock = new MockSystemClock();
	var serviceProvider = HubConnectionHandlerTestUtils.CreateServiceProvider(services =>
	services.Configure<HubOptions>(options =>
	options.KeepAliveInterval = TimeSpan.FromMilliseconds(interval)), LoggerFactory);
	var connectionHandler = serviceProvider.GetService<HubConnectionHandler<MethodHub>>();
	connectionHandler.SystemClock = clock;

	using (var client = new TestClient(new NewtonsoftJsonHubProtocol()))
	{
	var connectionHandlerTask = await client.ConnectAsync(connectionHandler);
	await client.Connected.DefaultTimeout();

	// Trigger multiple keep alives
	var heartbeatCount = 5;
	for (var i = 0; i < heartbeatCount; i++)
	{
	clock.UtcNow = clock.UtcNow.AddMilliseconds(interval + 1);
	client.TickHeartbeat();
	}

	await client.Connected.OrThrowIfOtherFails(connectionHandlerTask).DefaultTimeout();

	await client.BeginUploadStreamAsync("invocation", nameof(MethodHub.UploadDoesWorkOnComplete), streamIds: new[] { "id" }, args: Array.Empty<object>()).DefaultTimeout();

	await client.SendHubMessageAsync(new StreamItemMessage("id", "hello")).DefaultTimeout();

	await client.DisposeAsync().DefaultTimeout();

	await connectionHandlerTask.DefaultTimeout();

Conversation

TanayParikh commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

TanayParikh Jun 29, 2021 • 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!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

greenEkatherine commented Jun 29, 2021

Uh oh!

azure-pipelines Bot commented Jun 29, 2021

Uh oh!

SteveSandersonMS commented Jun 29, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TanayParikh commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TanayParikh commented Jun 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TanayParikh commented Jun 30, 2021

Uh oh!

azure-pipelines Bot commented Jun 30, 2021

Uh oh!

TanayParikh commented Jul 8, 2021

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TanayParikh commented Jun 28, 2021 •

edited

Loading

TanayParikh Jun 29, 2021 •

edited

Loading

TanayParikh commented Jun 29, 2021 •

edited

Loading

TanayParikh commented Jun 29, 2021 •

edited

Loading