-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Remove WebRequest from SoundPlayer #65854
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
Remove WebRequest from SoundPlayer #65854
Conversation
Replaced both usages of `WebRequest` and replaced with `HttpClient` instead.
Replaced the reference to `System.Net.Requests` with a reference to `System.Net.Http`.
Tagging subscribers to this area: @dotnet/ncl Issue DetailsI have removed the two instances of I could maybe refine it to only allocate a Fix #41516
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @KieranFoot
#pragma warning disable SYSLIB0014 // WebRequest, HttpWebRequest, ServicePoint, and WebClient are obsolete. Use HttpClient instead. | ||
WebRequest webRequest = WebRequest.Create(_uri); | ||
#pragma warning restore SYSLIB0014 | ||
webRequest.Timeout = LoadTimeout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we are loosing LoadTimeout
without replacement. We may consider something like:
using CancellationTokenSource cts = new CancellationTokenSource();
cts.CancelAfter(LoadTimeout);
and pass it to GetStreamAsync
|
||
// now get the stream | ||
_stream = webResponse.GetResponseStream(); | ||
_stream = _httpClient.GetStreamAsync(_uri).Result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_stream = _httpClient.GetStreamAsync(_uri).Result; | |
_stream = _httpClient.GetStreamAsync(_uri).GetAwaiter().GetResult(); |
Since this function is synchronous, I'm wondering if we should use HttpClient.Send
instead to avoid sync-over-async
Any thought on that @ManickaP @stephentoub ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpWebRequest
is using the sync APIs already so we should likely do so here as well, or this change would actually regress perf.
Be aware that initiating and using a new HttpClient per instance of the Soundplayer is not the suggested way to use it. |
I appreciate the intent of the PR, but I think it's actually taking steps backwards in a few ways:
It might make sense to wait for #65380 before trying to proceed here further. That would take care of the 1st issue, and the second issue can be addressed with just a bit more code (or we could also decide to add more sync helpers to HttpClient, e.g. a GetStream in addition to GetStreamAsync). |
@stephentoub I am working on refining the use of HttpClient to use the sync methods where applicable. As for your first point, I can see where HttpWebRequest has a cache of HttpClient, maybe that could be implemented at the HttpClient level. |
@KieranFoot are you still working on this? |
@MihaZupan I did do some more work that I can add to the PR, however I think there was a call to wait for the issue #65380 I'll push what I have and hand over to you :) |
We are looking at potentially implementing the |
Closing as per above until the API is available. |
I have removed the two instances of
WebRequest
and instead useHttpClient
.I could maybe refine it to only allocate a
HttpClient
if required.Fix #41516