dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.46k stars 4.76k forks source link

WebProxy.GetSystemProxy #95252

Open TonyValenti opened 1 year ago

TonyValenti commented 1 year ago

Background and motivation

When troubleshooting a live application, it would be nice to be able to open Fiddler and (nearly) immediately see HTTP requests flowing through it.

Unfortunately, the current HTTPClient / HttpClient.DefaultProxy is not suitable for this. The first time it is called it seems to capture the value from SystemProxyInfo.ConstructSystemProxy and cache is for the life of the application. This results in needing reflection or other methods in order to retrieve the actualy current system proxy information.

API Proposal

namespace System.Net.Http;

public static class WebProxy
{
    public static IWebProxy GetSystemProxy();
}

API Usage

// Fancy the value
var Proxy = System.Net.Http.WebProxy.GetSystemProxy();

Alternative Designs

Leave as-is.

Risks

I assume that SystemProxyInfo.ConstructSystemProxy is a heavy method. Users of this function might call it too frequently.

ghost commented 1 year ago

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

Issue Details
### Background and motivation When troubleshooting a live application, it would be nice to be able to open Fiddler and (nearly) immediately see HTTP requests flowing through it. Unfortunately, the current HTTPClient / HttpClient.DefaultProxy is not suitable for this. The first time it is called it seems to capture the value from SystemProxyInfo.ConstructSystemProxy and cache is for the life of the application. This results in needing reflection or other methods in order to retrieve the actualy current system proxy information. ### API Proposal ```csharp namespace System.Net.Http; public class WebProxy { public IWebProxy GetSystemProxy(); } ``` ### API Usage ```csharp // Fancy the value var Proxy = System.Net.Http.WebProxy.GetSystemProxy(); ``` ### Alternative Designs Leave as-is. ### Risks I assume that SystemProxyInfo.ConstructSystemProxy is a heavy method. Users of this function might call it too frequently.
Author: TonyValenti
Assignees: -
Labels: `api-suggestion`, `area-System.Net.Http`
Milestone: -
ManickaP commented 1 year ago

Just to be sure, you just want us to expose an existing, private method that creates a proxy based on environmental settings, so that you can manually set it when creating HttpClient? Because the default property is process wide, never refreshed static. If so, this seems reasonable, we can always document the function as heavy and leave up to the user to decide.

@wfurt any concerns here? BTW, why don't we reconstruct the proxy with HttpClient creation? I assume we don't expect the proxy settings to change?

Also tangentially related #35992

TonyValenti commented 1 year ago

@ManickaP - Yes. This is exactly what I want and the reason I want it.

If this was implemented, it would also be nice if there was an implementation of IWebProxy that would "follow" changes in the system web proxy.

In my own code, I have rolled the following implementations but I imagine these are common asks that many folks could leverage.

Here is my code:



namespace System.Net.Http {

    //Code to call the current SystemProxyInfo method via reflection
    public static class WebProxyFactory {
        static WebProxyFactory() {
            {
                var TypeName = "System.Net.Http.SystemProxyInfo";
                var Type = typeof(HttpClient).Assembly.GetType(TypeName);
                var MethodName = "ConstructSystemProxy";
                var Method = Type?.GetMethod(MethodName);
                var Delegate = Method?.CreateDelegate<Func<IWebProxy>>();

                if (Delegate is { }) {
                    Factory = Delegate;
                } else {
                    throw new MissingMethodException("Unable to find proxy functions");
                }
            }
            SystemProxy = new SystemWebProxy(TimeSpans.OneSecond);
        }

        private static Func<IWebProxy> Factory { get; }
        public static IWebProxy GetCurrentSystemProxy() {
            return Factory();
        }

        public static IWebProxy SystemProxy { get; }

    }

//An IWebProxy that will re-retrieve the proxy from the system every so often.
public class SystemWebProxy : DisplayClass, IWebProxy {
    public TimeSpan Timeout { get; }
    public DateTimeOffset ValidUntil { get; private set; }

    public override DisplayBuilder GetDebuggerDisplayBuilder(DisplayBuilder Builder) {
        return base.GetDebuggerDisplayBuilder(Builder)
            .Data.AddExpression(ValidUntil)
            ;
    }

    public SystemWebProxy(TimeSpan Timeout) {
        this.Timeout = Timeout;

        GetCurrentCache = GetCurrent();
    }

    private readonly object GetCurrentLock = new();
    private volatile IWebProxy GetCurrentCache;
    protected IWebProxy GetCurrent() {
        var ret = GetCurrentCache;

        if (ValidUntil < DateTimeOffset.Now) {
            lock (GetCurrentLock) {
                if (ValidUntil < DateTimeOffset.Now) {
                    ret = WebProxyFactory.GetCurrentSystemProxy();
                    GetCurrentCache = ret;
                    ValidUntil = DateTimeOffset.Now + Timeout;
                }
            }
        }
        return ret;

    }

    public ICredentials? Credentials { get => GetCurrent().Credentials; set => GetCurrent().Credentials = value; }

    public Uri? GetProxy(Uri destination) {
        return GetCurrent().GetProxy(destination);
    }

    public bool IsBypassed(Uri host) {
        return GetCurrent().IsBypassed(host);
    }
}

}
karelz commented 1 year ago

I think it is dupe of #46910

wfurt commented 1 year ago

I think the devil is in the detail @manickap. I don't think there is notification about changes. And in broader sense when auto configuration is done via script we don't even have visibility to what server shall be used. That can be completely custom logic and only one way to know is to execute javascript. (Windows only AFAIK)

ManickaP commented 1 year ago

I meant, whether we could re-call SystemProxyInfo.ConstructSystemProxy when we are setting up _proxy on the pool manager here: https://source.dot.net/#System.Net.Http/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs,131

What we do now is that we check the system settings once per the whole process, store the result in static and use it for the whole application lifetime.

I'm aware that automatically checking for system changes is nigh impossible.

wfurt commented 1 year ago

There are multiple threads/reasons and I think we should differentiate.

How is this API proposal different from WebRequest.GetSystemWebProxy? Both static methods giving back IWebProxy. If that one does not return fresh proxy value we can make it so IMHO without adding new API surface.

I would be supportive to query the proxy more often @ManickaP . We can do it when HttpClient is created. That is still difficult to use IMHO but I would see it as easy improvement.

I think we should do it on NetworkChange in 9.0. I certainly move my macBook around and I don't want to restart VS when connected to different network with different proxy. Reading it once on app start ignores current mobility IMHO.

I think we should investigate https://github.com/dotnet/runtime/issues/46910 and have proper fix. The claim there is that Framework worked so we should be able to figure it out. I'm not sure about macOS but that seems less pressing at the moment.

I feel https://github.com/dotnet/runtime/issues/35992 is different animal. While related, I don't think it is trying to make better experience with system setting.

ManickaP commented 1 year ago

WebRequest.GetSystemWebProxy does not re-fetch, it returns the static, once per-process initialized static field (that was the crux of #46910 - the original complaint).

I'll mark this for further triage and we can talk about the whole topic as a team.

ManickaP commented 10 months ago

Triage: we chatted about this issue and https://github.com/dotnet/runtime/issues/46910 together. We decided that the optimal solution would be to auto update the settings. In .NET Framework we had a mechanism to listen to proxy changes, so we could bring this back in for Windows (or use the suggested NetworkChanged hook by @wfurt to re-fetch). On Linux, re-fetching the environment variables, which are cached by the runtime, should be fast enough to do even with every connection construction. So the goal would not be to expose a new API, but rather change the HttpEnvironmentProxy behavior. This might change when we delve into the Windows behavior in more detail, but for now it seems like an ideal solution to both of these issues.

wfurt commented 10 months ago

Do we want to fix WebRequest.GetSystemWebProxy or do we want to move forward with new API @ManickaP? (or both or neither)

I personally feel #46910 is different animal and perhaps should be tracked independently. e.g. getting current OS value is much easier IMHO than tracking changes and applying them automatically.

ManickaP commented 10 months ago

What I thought we agreed is to change the behavior of HttpEnvironmentProxy class, which would in turn fix HttpClient.DefaultProxy and also WebRequest.GetSystemWebProxy, which just calls the client-one: https://github.com/dotnet/runtime/blob/dbb335c6ba14b38400b2d8c3a5876698021ec089/src/libraries/System.Net.Requests/src/System/Net/WebRequest.cs#L546

So we would keep the process-wide statics, but it would return an implementation that re-fetches on Linux and listens to changes on Windows (because it may potentially run running expensive JS code). This would also fix #46910. Unless I misunderstood that issue or our agreement.

stephentoub commented 10 months ago

because it may potentially run running expensive JS code

How do we plan to do that?

wfurt commented 10 months ago

Having platform differences is not great IMHO. While we agreed that lookup of environment variable is cheap, it is also unlikely to change IMHO. I was under impression that we agreed to:

1) Fix GetSystemWebProxy to return value and accurate value on each invocation (instead of adding new API) Since that points to HttpClient.DefaultProxy the actual fix would be there. It can remain static but when somebody would query it, it would return ether exising object or new instance if change was detected. For the implementations we know we can add some internal method like CreateOrUpdate(iWebProxy)

Creating new HttpClient should create/validate proxy setting. The JS may be evaluated as part of this creating new client is already not cheap. Under the cover all this handled in native code wrapped by WinInetProxyHelper. (and/or we can also move it to the handler under the cover as that may be even more rare)

This gives somebody crude tools to react to proxy changes and/or do their own queries to get current correct setting. And I think it would be cheap from execution and development prospective with minimal (IMHO) impact on existing users.

2) enhance existing code to deal automaticly with changes better. There can be whole bucket of improvements we can make and that can be done individually IMHO.

This make sense IMHO only for out own system proxies. If somebody already set default or instance to their own implementation if IWebProxy we should just leave it alone. Since it is all in same assembly we can leverage internal methods beyond IWEbProxy and for example we can communicate IO failures to the instance and kick extra processing

Here I feel we should not be too aggressive as it can be expensive. Fetching new setting on failure or trigger seems reasonable to me. To me, this is the answer to the how question above.

ManickaP commented 5 months ago

Triage: this should get somewhat alleviated with #103364, at least for Windows where it makes most sense. So punting to future for now.