Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] browser http response stream could be seekable #54603

Merged
merged 3 commits into from
Jun 25, 2021

Conversation

pavelsavara
Copy link
Member

As discussed here, the unit test is too restrictive.
Fixes #54159

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Net labels Jun 23, 2021
@pavelsavara pavelsavara requested review from lewing and geoffkizer June 23, 2021 11:50
@ghost
Copy link

ghost commented Jun 23, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

As discussed here, the unit test is too restrictive.
Fixes #54159

Author: pavelsavara
Assignees: -
Labels:

arch-wasm, area-System.Net

Milestone: -

@campersau
Copy link
Contributor

An additional test for when EnableStreamingResponse is set would be nice too, in this case the response stream wouldn't be seekable and sync reads would throw.

bool streamingEnabled = false;
if (StreamingSupported)
{
request.Options.TryGetValue(EnableStreamingResponse, out streamingEnabled);
}
httpResponse.Content = streamingEnabled
? new StreamContent(wasmHttpReadStream = new WasmHttpReadStream(status))
: (HttpContent)new BrowserHttpContent(status);

@@ -969,21 +968,24 @@ public async Task ReadAsStreamAsync_HandlerProducesWellBehavedResponseStream(boo
// Boolean properties returning correct values
Assert.True(responseStream.CanRead);
Assert.False(responseStream.CanWrite);
Assert.False(responseStream.CanSeek);
Assert.Equal(PlatformDetection.IsBrowser, responseStream.CanSeek);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this check after all? Hypothetically, when there's new platform with response stream that can seek as well, this line would need an update again. Or does it has some other purpose here?

@pavelsavara pavelsavara merged commit c139d00 into dotnet:main Jun 25, 2021
@adamsitnik
Copy link
Member

@pavelsavara it seems that this PR has broken the full Framework build (Libraries Build windows net48 x86 Release CI leg was red)

@pavelsavara
Copy link
Member Author

thanks Adam, I missed both the build failure and the #IF.
I'm on it, sorry.

pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Jun 25, 2021
@pavelsavara
Copy link
Member Author

Revert #54742

@pavelsavara
Copy link
Member Author

Next attempt in #54749

@karelz karelz added this to the 6.0.0 milestone Jul 15, 2021
@pavelsavara pavelsavara deleted the wasm_BrowserHttpContent_seekable branch July 29, 2021 08:46
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm][http] response.Content.ReadAsStreamAsync().CanSeek != false
7 participants