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

WebProxy.GetSystemProxy #95252

Open
TonyValenti opened this issue Nov 27, 2023 · 14 comments
Open

WebProxy.GetSystemProxy #95252

TonyValenti opened this issue Nov 27, 2023 · 14 comments
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@TonyValenti
Copy link

TonyValenti commented Nov 27, 2023

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.

@TonyValenti TonyValenti added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 27, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 27, 2023
@ghost
Copy link

ghost commented Nov 27, 2023

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

namespace System.Net.Http;

public class WebProxy
{
    public 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.

Author: TonyValenti
Assignees: -
Labels:

api-suggestion, area-System.Net.Http

Milestone: -

@ManickaP
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

karelz commented Nov 28, 2023

I think it is dupe of #46910

@wfurt
Copy link
Member

wfurt commented Nov 28, 2023

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
Copy link
Member

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
Copy link
Member

wfurt commented Nov 29, 2023

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 #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 #35992 is different animal. While related, I don't think it is trying to make better experience with system setting.

@ManickaP
Copy link
Member

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 ManickaP added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Nov 29, 2023
@ManickaP ManickaP added this to the 9.0.0 milestone Jan 16, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 16, 2024
@ManickaP ManickaP added enhancement Product code improvement that does NOT require public API changes/additions and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration labels Jan 16, 2024
@ManickaP
Copy link
Member

Triage: we chatted about this issue and #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.

@ManickaP ManickaP changed the title [API Proposal]: WebProxy.GetSystemProxy WebProxy.GetSystemProxy Jan 16, 2024
@ManickaP ManickaP self-assigned this Jan 16, 2024
@wfurt
Copy link
Member

wfurt commented Jan 18, 2024

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
Copy link
Member

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:

public static IWebProxy GetSystemWebProxy() => HttpClient.DefaultProxy;

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
Copy link
Member

because it may potentially run running expensive JS code

How do we plan to do that?

@wfurt
Copy link
Member

wfurt commented Jan 24, 2024

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.

  1. 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.
  • WinInetProxyHelper already has RecentAutoDetectionFailure. We can extend this to SocketsHttpHandler
    and re-fecth proxies (occationally) if we fail to connect to proxy server
  • we can use network change monitoring (since already have hook for HTTP/3)
  • we can explore Framework and see if they do anything above and beyond Core. As minimum, I think we sho would should be able to detect registry changes.

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
Copy link
Member

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

@ManickaP ManickaP modified the milestones: 9.0.0, Future Jun 13, 2024
@ManickaP ManickaP removed their assignment Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants