Skip to content

Make HttpWebRequest.GetRequestStream return same instance (fixes #41403) #116757

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 20, 2025
Merged

Make HttpWebRequest.GetRequestStream return same instance (fixes #41403) #116757

merged 5 commits into from
Jun 20, 2025

Conversation

Lukeuke
Copy link
Contributor

@Lukeuke Lukeuke commented Jun 17, 2025

Summary

Fixes a compatibility issue where HttpWebRequest.GetRequestStream() returned a new stream instance on each call, unlike .NET Framework which returns the same instance.

Changes

  • Added null coalescing operators on _requestStream inside InternalGetRequestStream
  • Added test GetRequestStream_ReturnsSameInstanceWithoutLoopback.

Fixes: #41403

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 17, 2025
This was referenced Jun 17, 2025
Closed
Open
Closed
This was referenced Jun 18, 2025
Open
Closed
liveans
liveans reviewed Jun 19, 2025
View reviewed changes
Copy link
Member

@liveans liveans left a comment

Choose a reason for hiding this comment

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

First of all, thanks for the contribution! This seems mostly ok, however we need to add async path test + fix up the async path, to have the same behavior with sync path.

CI Failures look unrelated, I re-triggered it.

@Lukeuke
Copy link
Contributor Author

Lukeuke commented Jun 19, 2025

First of all, thanks for the contribution! This seems mostly ok, however we need to add async path test + fix up the async path, to have the same behavior with sync path.

CI Failures look unrelated, I re-triggered it.

I've run the same test but async and it seems to be ok. Doesn't that mean the async path will have same behavior?
GetRequestStreamAsync is invoking BeginGetRequestStream and it is calling the InternalGetRequestStream so it will return same object? Correct me if I'm wrong

@liveans
Copy link
Member

liveans commented Jun 19, 2025

First of all, thanks for the contribution! This seems mostly ok, however we need to add async path test + fix up the async path, to have the same behavior with sync path.
CI Failures look unrelated, I re-triggered it.

I've run the same test but async and it seems to be ok. Doesn't that mean the async path will have same behavior? GetRequestStreamAsync is invoking BeginGetRequestStream and it is calling the InternalGetRequestStream so it will return same object? Correct me if I'm wrong

System.Net.Tests.HttpWebRequestTest_Async.GetRequestStream_ReturnsSameInstanceWithoutLoopback [FAIL]
        System.InvalidOperationException : Cannot re-call BeginGetRequestStream/BeginGetResponse while a previous call
  is still in progress.

This is what I get when I convert the test to async, for the context: HttpWebRequest_Sync and HttpWebRequest_Async only affects GetResponseAsync, and even we execute the Async test suite here, new test is still using Sync path.

@Lukeuke
Copy link
Contributor Author

Lukeuke commented Jun 19, 2025 •
edited
Loading

First of all, thanks for the contribution! This seems mostly ok, however we need to add async path test + fix up the async path, to have the same behavior with sync path.
CI Failures look unrelated, I re-triggered it.

I've run the same test but async and it seems to be ok. Doesn't that mean the async path will have same behavior? GetRequestStreamAsync is invoking BeginGetRequestStream and it is calling the InternalGetRequestStream so it will return same object? Correct me if I'm wrong

System.Net.Tests.HttpWebRequestTest_Async.GetRequestStream_ReturnsSameInstanceWithoutLoopback [FAIL]
        System.InvalidOperationException : Cannot re-call BeginGetRequestStream/BeginGetResponse while a previous call
  is still in progress.

This is what I get when I convert the test to async, for the context: HttpWebRequest_Sync and HttpWebRequest_Async only affects GetResponseAsync, and even we execute the Async test suite here, new test is still using Sync path.

I've double checked. I had stashed changes that are fixing that error. My bad.

@Lukeuke
Fixed GetRequestStreamAsync path to match sync version and added GetR…
bc50078 
…equestStream_ReturnsSameInstanceWithoutLoopback_Async test
This was referenced Jun 19, 2025
Closed
Open
liveans
liveans approved these changes Jun 19, 2025
View reviewed changes
stephentoub
stephentoub reviewed Jun 19, 2025
View reviewed changes
src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
@@ -1146,6 +1152,8 @@ public override IAsyncResult BeginGetRequestStream(AsyncCallback? callback, obje
throw new InvalidOperationException(SR.net_repcall);
}

Interlocked.Exchange(ref _endGetRequestStreamCalled, false);
Copy link
Member

Choose a reason for hiding this comment

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

@liveans, why is the code tracking whether begin/end were called?

Copy link
Member

Choose a reason for hiding this comment

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

why is the code tracking whether begin/end were called?

I'm not sure about the historical reason, but code was in that way since I've started to work on it. So, I didn't try to break the existing functionality.

My guess is that: It was due to APM functionality, and there are tests for it in the test suite as well.
But since people should be using Task-based functions instead, I think we should be safe to delete them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it means that these 4 fields should be safe to delete?

private bool _beginGetRequestStreamCalled;
private bool _beginGetResponseCalled;
private bool _endGetRequestStreamCalled;
private bool _endGetResponseCalled;

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, we can, but I'd rather prefer to keep it as-is right now. We might consider removing it later. I'll file an issue for it.

This was referenced Jun 19, 2025
Closed
Closed
Closed
Closed
@liveans
Copy link
Member

liveans commented Jun 20, 2025

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liveans liveans enabled auto-merge (squash) June 20, 2025 20:58
@liveans liveans disabled auto-merge June 20, 2025 20:59
@liveans
Copy link
Member

liveans commented Jun 20, 2025

Thanks for the contribution @Lukeuke, can you accept the CLA as described in the second comment? So, we can merge it.

@Lukeuke
Copy link
Contributor Author

Lukeuke commented Jun 20, 2025

@dotnet-policy-service agree

@Lukeuke
Copy link
Contributor Author

Lukeuke commented Jun 20, 2025

Thanks for the contribution @Lukeuke, can you accept the CLA as described in the second comment? So, we can merge it.

Accepted! Thanks for the opportunity to contribute and your time. It was new experience to me. I hope that it won't be the last.

@liveans liveans merged commit 2e24b81 into dotnet:main Jun 20, 2025
79 of 94 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@liveans liveans liveans approved these changes

Assignees
No one assigned
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HttpWebRequest.GetRequestStream returns new stream instance on every invocation
3 participants
@Lukeuke @liveans @stephentoub