-
Notifications
You must be signed in to change notification settings - Fork 4.9k
HttpWebRequest caches HttpClient in simple cases #41462
HttpWebRequest caches HttpClient in simple cases #41462
Conversation
if (rcvc != null) | ||
{ | ||
RemoteCertificateValidationCallback localRcvc = rcvc; | ||
handler.ServerCertificateCustomValidationCallback = (message, cert, chain, errors) => localRcvc(request, cert, chain, errors); |
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.
I'm not understanding the logic around request
being passed in to CreateHttpClient. null
is passed in when creating the cached instance, and the instance is passed in otherwise. That means then that for the cached instance, this request
will be null. And that means that anyone using the cached HttpClient instance will have a null request object passed into their RemoteCertificateValidationCallback Seems like a bug.
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.
Also, there's a subtle change here that's going to result in the closure being created even when the ServerCertificateValidationCallback is null. Previously the only state being closed over were locals, this, or confined to the immediate containing scope. This change now also closes over request
, which has method scope. That means the closure for this lambda would previously have only been allocated with rcvc != null, but now it's going to be allocated every time this method is called.
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.
-
Passing null is intentional because client caching is only allowed when ServiceCertificateValidationCallback == null, thus it will never be caught in the lambda. Additionally, even if somebody accidentally change caching conditions to allow validation callback caching, passing null will still protect from leaking of "this" instance in the static cache.
-
Valid point. I will fix it.
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
Most of the Outerloop failures are not related to the PR. However, the UWP failures are related to PR: The UWP tests need to probably be adjusted accordingly. |
- Code style fixes
- Code style fixes
- Code style fixes
- Code style fixes
Extra closure allocation eliminated Test verification logic changed
fccee22
to
14bec02
Compare
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
HttpWebRequest caches and tries to reuse a single static HttpClient instance when it's safe to share the same instance among concurrent requests with the given parameters. Fixes dotnet/corefx#15460 Commit migrated from dotnet/corefx@95e35e1
HttpWebRequest caches and tries to reuse a single static HttpClient instance when it's safe to share the same instance among concurrent requests with the given parameters.
Fixes #15460