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

[browser][http] Set HttpResponseMessage.RequestMessage #42822

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

campersau
Copy link
Contributor

Fixes #42691

@ghost
Copy link

ghost commented Sep 28, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@lewing lewing requested a review from kjpou1 September 28, 2020 20:06
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

Looks good

@lewing lewing added the arch-wasm WebAssembly architecture label Sep 28, 2020
@stephentoub
Copy link
Member

stephentoub commented Sep 28, 2020

Are we running our HTTP tests in wasm? I'm wondering if we have a test hole here, or if we do in fact have a test that would catch this but it didn't because we're not running them.

@lewing
Copy link
Member

lewing commented Sep 28, 2020

wasm Http tests in CI are waiting on #40786 but we've done recent manual runs that did not catch this so I suspect a test hole.

@stephentoub
Copy link
Member

but we've done recent manual runs that did not catch this so I suspect a test hole.

Hmm. I just searched the tests. It's true that there aren't any dedicated to this, and I can fix that. But there are a bunch of tests that just assume this is non-null and would null ref if it was, so something about those manual runs must have skipped all those tests.

@lewing
Copy link
Member

lewing commented Sep 29, 2020

Agreed, we're standing up the tests as quickly as possible.

@kjpou1 please look into why the tests aren't catching this.

@lewing
Copy link
Member

lewing commented Sep 29, 2020

/backport release/5.0

@lewing
Copy link
Member

lewing commented Sep 29, 2020

The issue with the testing here is that browser can't support the loopback server because we don't have sockets and can't listen for requests so all the test that require the loopback server will throw PNSE and have to be skipped.

@stephentoub
Copy link
Member

so all the test that require the loopback server will throw PNSE and have to be skipped.

Oookeeyyy... that's the vast majority of the functional tests ;-)

@lewing
Copy link
Member

lewing commented Sep 29, 2020

so all the test that require the loopback server will throw PNSE and have to be skipped.

Oookeeyyy... that's the vast majority of the functional tests ;-)

Yeah... I think we're the first platform to hit this but I don't think we will be the last.

@lewing
Copy link
Member

lewing commented Sep 29, 2020

Opened #42852 for discussion

@lewing
Copy link
Member

lewing commented Sep 29, 2020

Lane failure looks like jaredpar/runfo#47 and this change is browser specific

@lewing lewing merged commit c3171f5 into dotnet:master Sep 29, 2020
@campersau campersau deleted the browserrequestmessage branch September 29, 2020 19:12
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@karelz karelz added this to the 6.0.0 milestone Jan 26, 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.Http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blazor WASM: HttpClient.SendAsync does not set the RequestMessage in ResponseMessage
5 participants