Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Support PAC scripts that return multiple proxies #40082

Merged
merged 16 commits into from
Sep 5, 2019
Merged

Support PAC scripts that return multiple proxies #40082

merged 16 commits into from
Sep 5, 2019

Conversation

scalablecory
Copy link

@scalablecory scalablecory commented Aug 7, 2019 •
edited
Loading

This enables SocketsHttpHandler support for PAC scripts that return multiple eligible proxies for a single URL.

If a single proxy is returned, the old proxy path is followed.

If multiple proxies are returned, they will be used for failover in left-to-right order. A request finally fails after it has tried each proxy.
When a proxy fails, it will be marked as a failed proxy for 30 minutes, which is what WinHTTP and Firefox do. If all proxies have failed, it will try using the proxy that would have its 30 minutes elapse the soonest.

Resolves #39370.

@scalablecory scalablecory added this to the 5.0 milestone Aug 7, 2019
@scalablecory scalablecory requested review from a team, davidsh and stephentoub August 7, 2019 00:29
@scalablecory scalablecory self-assigned this Aug 7, 2019
davidsh
davidsh reviewed Aug 7, 2019
View reviewed changes
Copy link
Contributor

@davidsh davidsh left a comment

Choose a reason for hiding this comment

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

Did a first pass on this for style things.

@stephentoub
Copy link
Member

if one request fails over and has success on the 2nd proxy, subsequent requests will begin by using the 2nd proxy

Is this how other clients do it? Just want to make sure we're following precedent (or a spec if there happens to be one somewhere).

stephentoub
stephentoub reviewed Aug 7, 2019
View reviewed changes
stephentoub
stephentoub reviewed Aug 7, 2019
View reviewed changes
@davidsh
Copy link
Contributor

davidsh commented Aug 7, 2019

Is this how other clients do it? Just want to make sure we're following precedent (or a spec if there happens to be one somewhere).

We should ask the WinInet/WinHttp team about this. They can tell us the algorithm they use.

@scalablecory
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@scalablecory
Copy link
Author

@stephentoub @davidsh @dotnet/ncl ready for review, thanks.

davidsh
davidsh reviewed Aug 28, 2019
View reviewed changes
davidsh
davidsh approved these changes Sep 4, 2019
View reviewed changes
Copy link
Contributor

@davidsh davidsh left a comment

Choose a reason for hiding this comment

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

Left a comment about the use of the 'Collection' suffix for a class that isn't a .NET collection type.

@davidsh
Copy link
Contributor

davidsh commented Sep 4, 2019

Also, please run final Outerloop tests before merge.

@scalablecory
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@scalablecory
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@scalablecory
Copy link
Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@scalablecory scalablecory merged commit 5e97c2b into dotnet:master Sep 5, 2019
@scalablecory scalablecory deleted the 39370-multi-pac branch September 5, 2019 16:41
stephentoub
stephentoub reviewed Sep 19, 2019
View reviewed changes
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/FailedProxyCache.cs

// This lock can be folded into _nextFlushTicks for space optimization, but
// this class should only have a single instance so would rather have clarity.
private SpinLock _flushLock = new SpinLock();
Copy link
Member

Choose a reason for hiding this comment

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

@scalablecory, I only just noticed this. Why did you choose to use a spin lock here rather than a regular lock?

stephentoub
stephentoub reviewed Sep 19, 2019
View reviewed changes
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/FailedProxyCache.cs
/// so it's only run periodically and is disabled until we exceed <see cref="LargeProxyConfigBoundary"/> failed proxies.
/// </remarks>
[MethodImpl(MethodImplOptions.NoInlining)]
private void CleanupHelper()
Copy link
Member

Choose a reason for hiding this comment

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

Presumably we expect only a relatively small number of proxies to end up as failed? Instead of looping through, storing times, etc., did you consider instead just creating a timer that will remove the proxy from the dictionary when it fires? That seems like it'd be a lot simpler.

stephentoub
stephentoub reviewed Sep 19, 2019
View reviewed changes
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/FailedProxyCache.cs
/// Cleans up any old proxies that should no longer be marked as failing.
/// </summary>
/// <remarks>
/// I expect this to never be called by <see cref="Cleanup"/> in a production system. It is only needed in the case
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I generally prefer code to not use "I". When I read this code in 6 months, or a year, or a few years, who is "I"... it means I need to go and figure out who the developer was who committed the code. I'd personally prefer it if it just said "We" or if it was written to avoid the need for a pronoun, e.g. "This code is unlikely to ever be called in a production system."

stephentoub
stephentoub reviewed Sep 19, 2019
View reviewed changes
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/FailedProxyCache.cs
/// that a system has a very large number of proxies that the PAC script cycles through. It is moderately expensive,
/// so it's only run periodically and is disabled until we exceed <see cref="LargeProxyConfigBoundary"/> failed proxies.
/// </remarks>
[MethodImpl(MethodImplOptions.NoInlining)]
Copy link
Member

Choose a reason for hiding this comment

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

This attribute should not be necessary. There's a lot of code here, such that the JIT wouldn't generally want to inline it, and even if those heuristics changed, it's got exception handling that would prevent it as well.

stephentoub
stephentoub reviewed Sep 19, 2019
View reviewed changes
src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/FailedProxyCache.cs
try
{
_flushLock.TryEnter(ref lockTaken);
if (!lockTaken)
Copy link
Member

@stephentoub stephentoub Sep 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Especially with this logic there's no reason to use a spin lock, as this code is saying to not spin/block at all. The code just use Monitor.TryEnter on this. I suspect you'll find SpinLock is more expensive given the ctor that was used to create it.

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@scalablecory
Support PAC scripts that return multiple proxies (dotnet/corefx#40082)
b0c6eb7 
* Support PAC scripts that return multiple proxies.

Resolves dotnet/corefx#39370 

Commit migrated from dotnet/corefx@5e97c2b
@wfurt wfurt mentioned this pull request Jun 25, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@stephentoub stephentoub stephentoub left review comments

@eiriktsarpalis eiriktsarpalis eiriktsarpalis left review comments

@davidsh davidsh davidsh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@scalablecory scalablecory

Labels

area-System.Net.Http

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

Cycle through proxies when unable to connect and PAC returns multiple proxies.

4 participants

@scalablecory @stephentoub @davidsh @eiriktsarpalis