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

Use HttpClient instead of HttpWebRequest - breaking change #1406

Closed
alexeyzimarev opened this issue Jan 14, 2020 · 29 comments
Closed

Use HttpClient instead of HttpWebRequest - breaking change #1406

alexeyzimarev opened this issue Jan 14, 2020 · 29 comments
Labels
Milestone

Comments

@alexeyzimarev
Copy link
Member

Currently, RestSharp uses HttpWebRequest, since that was the "way to do it" back in the days.

Time passed by and HttpClient dominates now, so we need to move on.

There are several issues with HttpWebRequest implementation that could be resolved by using HttpClient:

  • Use proper async flow with cancellation and context
  • Use a single instance of HttpClient for connection pooling
  • Support HTTP2

These are the main issues but there're advantages that overall come with more modern implementation.

In the latest versions of .NET framework (full and core), HttpWebRequest uses the HttpClient anyway.

@lrhoads
Copy link

lrhoads commented Jan 16, 2020

The issue is larger, in that a new instance of HttpClient is created for each RestClient, which is leading to possible port exhaustion. In .NET Core, each new instance of HttpWebRequest will create a new HttpClient, rather than using a static.

I have opened an issue with .NET Core, but it seems unlikely they are going to fix despite it being a minor change. Reference Issue: dotnet/runtime#1812.

@scalablecory
Copy link

I'd love to see RestSharp get this update. @alexeyzimarev, please reach out if there's any questions on HttpClient.

@TeunKooijman
Copy link

I would also like to vouch for enabling the injection of an HttpClient into an IRestClient, so that we can use the clients that are being set up by TestHost in .NET Core integration tests, e.g.:

public class MyIntegrationTests : IClassFixture<WebApplicationFactory<Startup>>
{
     private WebApplicationFactory<Startup> _factory;

     public MyIntegrationTests(WebApplicationFactory<Startup> factory)
     {
           this._factory = factory;
     }

     [Fact]
     public void SomeTest()
     {
           HttpClient httpClient = this._factory.CreateClient();
           IRestClient restClient = new RestClient(httpClient);

           //or
           IRestClient restClient = new RestClient();
           restClient.HttpClient = _factory.CreateClient();

           //or perhaps something more in line with .NET Core's DI fascilities.
     }
}

@alexeyzimarev alexeyzimarev added this to the v107 milestone May 8, 2020
@alexeyzimarev alexeyzimarev changed the title Support HttpClient in addition to HttpWebRequest Use HttpClient instead of HttpWebRequest - breaking change May 8, 2020
@alexeyzimarev
Copy link
Member Author

@TeunKooijman it won't be that straightforward.

As part of the HttpClient implementation, I need to apply the RestClient configuration properties to the HttpMessageHandler instance. Each HttpClient instance must use that handler instance, otherwise, the settings won't get applied. But it means that when instantiating HttpClient, I need to pass the handler as the constructor parameter.

If we allow setting the HttpClient from the outside of RestClient, none of the RestClient settings will apply to that client and it won't really test anything.

@yilezhu
Copy link

yilezhu commented Jun 28, 2020

Hello, friend, how is it going?

@snuffykl
Copy link

snuffykl commented Jul 14, 2020

Hi @alexeyzimarev, I wanted to know the progress of introducing to use HttpClient in RestSharp ?

@alexeyzimarev
Copy link
Member Author

Some code is in the branch. The issue I am facing is how to make the API cleaner. Both IRestClient and IRestRequest are overloaded with properties that influence how requests are being made. There will be a lot of breaking changes, that's the only thing I can promise... In addition, not everything that is supported (or claimed) by HttpRequest is available in HttpClient.

@alexeyzimarev
Copy link
Member Author

In addition, I got a bit stuck on the HttpClientFactory thing. It was added to address exactly the same concerns as RestSharp users have about connection pooling. Each client has its own configuration and it is impossible to share the single client, even between requests, since some RestSharp settings need to be moved to the HttpMessageHandler, so the client needs to change.

There are two solutions there:

  1. Try to do what HttpCleintFactory is doing and cache the handler instances
  2. Do not allow changing any settings that would require creating a new HttpMessageHandler instance in IRestRequest.

I would prefer the second option but it is very restrictive.

@jmalczak
Copy link

Is there any workaround for this issue except rewrite to httpclientfactory?

@alexeyzimarev
Copy link
Member Author

I studied the code of HttpWebRequest and they do cache instances of HttpMessageHandler (which is the source of the connection pool exhaustion, it's not the client itself). The compare the handler settings by value, so if any setting from the set is different from what they have in the cache, they create a new handler.

Essentially, it should work as-is already now. The problem that I see is that both IRestClient and IRestRequest have a bunch of setting and it's hard to figure out what setting will trigger the creation of a new HttpMessageHandler. My plan is to restructure those settings so it becomes apparent when the new handler will be created.

Remember that since RestSharp has no access to HttpClientFactory, it will need to implement the same pattern as Microsoft does. So, we will cache existing instances of HttpMessageHandler and still will create new instances if the settings don't match. Ideally, none of IRestRequest settings should force us to create the new handler instance. By doing so, we can use a single instance of the handler for a single RestClient instance.

This PR dotnet/corefx#41462 mentioned in #1322 introduced the handler caching. If you stick to only changing the settings that are used to determine if the new handler is needed, you won't encounter the issue.

@jmalczak
Copy link

Hey, thanks for the reply. So just to confirm if I use restrequest that doesn't set parameters checked here AreParametersAcceptableForCaching (method from PR) restsharp will cache httpmessagehandler. So in this case I can freely instantiate new RestClient in my service methods or I should cache RestClient instance by myself anyway?

@alexeyzimarev
Copy link
Member Author

RestSharp caches nothing. That's how HttpWebRequest works.

@jmalczak
Copy link

Got it, but what's the preferred usage of restclient then, let's say that I have 10 services instantiated by di container, all of them send json payload and doesn't set anything specific on restclient. Should I have one instance of RestClient provided to those 10 services or 10 instances of restclient so each service has it's own instance?

@alexeyzimarev
Copy link
Member Author

It's less about how many instances of something you have that uses RestClient. It is more about isolating an external API in a typed client and use it as a singleton, reusing the RestClient instance. RestClient supposes to be thread-safe. But, ten instances is not a problem, the issue is when a new HttpMessageHandler instance is created for each request.

@jmalczak
Copy link

Thank you!

@rezalas
Copy link
Contributor

rezalas commented Aug 27, 2020

@alexeyzimarev Are you still needing help on this, or do you have it covered?

@alexeyzimarev
Copy link
Member Author

I have a branch that I can share. The struggle is around the set of settings that would require creating a new HttpMessageHandler instance. The rest is quite trivial.

@bluetentacle
Copy link

Hi, we are having big problems with this also. When are you expecting the release to be out?

@mycomycul
Copy link

It's been about a month so I'm just checking if there are any updates on this matter. I would love to keep with RestSharp instead of switching all my code over to the HTTPClientFactory.

Is there a branch accessible with what's been done so far?

Thanks for all your hard work.

@zachrybaker
Copy link

@alexeyzimarev can you give us a time window on this?

@alexeyzimarev
Copy link
Member Author

@zachrybaker sorry, but no, I can’t. I have a lid job and after half a year since RestSharp has sponsorship enabled, no one is willing to contribute depicts the package is being downloaded tens of thousands times per day. I can do charity for just as much, but no more.

@james-s-tayler
Copy link

@zachrybaker @mycomycul @bluetentacle if you're not precious about RestSharp as a dependency, Refit has full support HttpClient and HttpClientFactory and is a reasonably mature library.

I noticed when doing an integration with Xero their official netstandard SDK is generated using the OpenAPI csharp-netcore generator which uses RestSharp under the hood. I have a rule to only use 3rd party SDKs if they allow our engineers full control over the HTTP requests by using HttpClient, so went with a hybrid solution where we used the models generated by the OpenAPI tooling, but coupled that with a Refit client which lets us use HttpClient.

The ability to add a custom pipeline of DelegatingHandler's to do logging/tracing/metrics/exception handling/rate-limiting/fault-tolerance/auth in a way that is completely decoupled from the actual client itself has been quite a game changer in terms of maintainability and stability of our integrations. It doesn't make sense to not be using HttpClient in 2020. I'm glad to see support for it in RestSharp is under consideration, as I know this library still has a large install-base and a large part of the mindshare as the go-to solution due to its long-standing popularity.

@zachrybaker
Copy link

zachrybaker commented Nov 6, 2020 via email

@nruffing
Copy link

Is there a branch with the in-progress work? Is any assistance needed?

I am also curious as to what breaking changes may be expected to IRestClient, IRestRequest and IRestResponse.

@stale
Copy link

stale bot commented Mar 31, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 31, 2021
@repo-ranger
Copy link

repo-ranger bot commented Mar 31, 2021

⚠️ This issue has been marked wontfix and will be closed in 3 days

@repo-ranger repo-ranger bot closed this as completed Apr 3, 2021
@tonyqus
Copy link

tonyqus commented May 10, 2021

Looks this issue should be kept open. Anyway, I understand the author may not give a exact deadline on this issue since he has a full time job. And I do appreciate the great work made by the author. @alexeyzimarev

@Christian-Oleson
Copy link

Coming here to say that this is an important change, as right now RestSharp will not work in Blazor web assembly.

@devmanzur
Copy link

although its irrelevant to Rest Sharp, I came across this same limitation while doing integration tests, so i instead wrote a HTTP client extension that kind of behaves like RestSharp. Hope someone finds it useful.

public static class HttpClientExtensions
{
    private static string BuildQuery(List<(string Key, string Value)> parameters)
    {
        var sb = new StringBuilder();

        foreach (var parameter in parameters)
        {
            if (sb.Length != 0)
            {
                sb.Append('&');
            }

            sb.Append($"{parameter.Item1}={parameter.Item2}");
        }

        return sb.ToString();
    }

    public static async Task<T> MakePostRequestAsync<T>(this HttpClient httpClient, RequestModel requestModel)
    {
        httpClient.DefaultRequestHeaders.Clear();
        requestModel.Headers.ForEach(x => httpClient.DefaultRequestHeaders.Add(x.Key, x.Value));
        var queryParameter = BuildQuery(requestModel.Parameters);

        var response =
            await httpClient.PostAsync($"{requestModel.Url}?{queryParameter}",
                new StringContent(requestModel.JsonBody, Encoding.UTF8, "application/json"));
        if (response.IsSuccessStatusCode)
        {
            return await response.Content.ReadFromJsonAsync<T>() ?? throw new Exception("Failed to parse response");
        }

        var error = await response.Content.ReadAsStringAsync();
        throw new Exception($"Api returned error response, Error: {error}");
    }
    
    public static async Task<T> MakeEncodedPostRequestAsync<T>(this HttpClient httpClient, RequestModel requestModel)
    {
        httpClient.DefaultRequestHeaders.Clear();
        requestModel.Headers.ForEach(x => httpClient.DefaultRequestHeaders.Add(x.Key, x.Value));
        // var queryParameter = BuildQuery(request.Parameters);

        var encodedParams = new Dictionary<string, string>();
        requestModel.Parameters.ForEach(x => encodedParams.Add(x.Key, x.Value));
        httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

        var response =
            await httpClient.PostAsync(requestModel.Url,
                new FormUrlEncodedContent(encodedParams));
        
        if (response.IsSuccessStatusCode)
        {
            return await response.Content.ReadFromJsonAsync<T>() ?? throw new Exception("Failed to parse response");
        }

        var error = await response.Content.ReadAsStringAsync();
        throw new Exception($"Api returned error response, Error: {error}");
    }
    
    public static async Task<T> MakeGetRequestAsync<T>(this HttpClient httpClient, RequestModel requestModel)
    {
        httpClient.DefaultRequestHeaders.Clear();
        requestModel.Headers.ForEach(x => httpClient.DefaultRequestHeaders.Add(x.Key, x.Value));
        var queryParameter = BuildQuery(requestModel.Parameters);

        var response =
            await httpClient.GetAsync($"{requestModel.Url}?{queryParameter}");
        if (response.IsSuccessStatusCode)
        {
            return await response.Content.ReadFromJsonAsync<T>() ?? throw new Exception("Failed to parse response");
        }

        throw new Exception("Api returned error response");
    }

    public static async Task<T> MakeDeleteRequestAsync<T>(this HttpClient httpClient, RequestModel requestModel)
    {
        httpClient.DefaultRequestHeaders.Clear();
        requestModel.Headers.ForEach(x => httpClient.DefaultRequestHeaders.Add(x.Key, x.Value));
        var queryParameter = BuildQuery(requestModel.Parameters);

        var response =
            await httpClient.DeleteAsync($"{requestModel.Url}?{queryParameter}");
        if (response.IsSuccessStatusCode)
        {
            return await response.Content.ReadFromJsonAsync<T>() ?? throw new Exception("Failed to parse response");
        }

        throw new Exception("Api returned error response");
    }
}


public class RequestModel
{
    public List<(string Key, string Value)> Headers { get; private set; }
    public List<(string Key, string Value)> Parameters { get; private set; }

    public string JsonBody { get; private set; }

    public string Url { get; private set; }

    public RequestModel(string url)
    {
        Url = url;
        Headers = new List<(string Key, string Value)>();
        Parameters = new List<(string Key, string Value)>();
    }

    public RequestModel AddHeader(string headerKey, string headerValue)
    {
        Headers.Add(new(headerKey, headerValue));
        return this;
    }

    public RequestModel AddParameter(string paramKey, string paramValue)
    {
        Parameters.Add(new(paramKey, paramValue));
        return this;
    }

    public RequestModel AddJsonBody<T>(T body) where T : class
    {
        JsonBody = JsonSerializer.Serialize(body);
        return this;
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests