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

some small proxy-related fixes #50770

Merged
merged 6 commits into from
Apr 11, 2021
Merged

Conversation

geoffkizer
Copy link
Contributor

Don't apply the max connections limit for ProxyConnect connections (i.e. the CONNECT we make to the proxy to create a proxy tunnel). The max connections limit is still applied for the user of the tunnel, which is where it makes sense.

Also, fix some logic with NT proxy auth where we could have tried to perform proxy authentication against an origin server if the origin server sent us a 407 (proxy auth challenge). The logic is now consistent with the HTTP proxy auth.

Add relevant tests and do some test cleanup.

Fixes #48363

@ghost
Copy link

ghost commented Apr 6, 2021

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

Issue Details

Don't apply the max connections limit for ProxyConnect connections (i.e. the CONNECT we make to the proxy to create a proxy tunnel). The max connections limit is still applied for the user of the tunnel, which is where it makes sense.

Also, fix some logic with NT proxy auth where we could have tried to perform proxy authentication against an origin server if the origin server sent us a 407 (proxy auth challenge). The logic is now consistent with the HTTP proxy auth.

Add relevant tests and do some test cleanup.

Fixes #48363

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

}

[Fact]
public async Task ProxyTunnelRequest_MaxConnectionsSetButDoesNotApplyToProxyConnect_Success()
Copy link
Member

Choose a reason for hiding this comment

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

Should we set the pool idle timeout to infinite here? I assume the failure condition for this test relies on

public static readonly TimeSpan DefaultPooledConnectionIdleTimeout = TimeSpan.FromMinutes(2);

being larger than

private static readonly TimeSpan s_defaultTimeout = TimeSpan.FromSeconds(100);

otherwise it could pass even without this change (after the first pool expired)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We accept both connections before sending the responses. So it shouldn't matter.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer
Copy link
Contributor Author

There are a bunch of CI legs that have been queued for a very long time. @dotnet/dnceng any idea what's up here?

@michellemcdaniel
Copy link
Contributor

Hi @geoffkizer. If these are windows jobs, they are likely related to the issues with the buildpool.windows.10.amd64.vs2019.open scale set that we are having. Issue is tracked here and here.

@geoffkizer
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer geoffkizer merged commit c5872ac into dotnet:main Apr 11, 2021
@geoffkizer geoffkizer deleted the proxyconnectfixes branch April 11, 2021 08:40
@ghost ghost locked as resolved and limited conversation to collaborators May 11, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectionPool exhaustion when using Proxy and MaxConnectionsPerServer
5 participants