it takes up a large amount of CPU when a large number of concurrent accesses, cause of using non static regex veriable in method RestClient.AddHandler

[guke001]

#if SILVERLIGHT
private readonly Regex structuredSyntaxSuffixRegex = new Regex(@"+\w+$");

    private readonly Regex structuredSyntaxSuffixWildcardRegex = new Regex(@"^\*\+\w+$");

#else
private readonly Regex structuredSyntaxSuffixRegex = new Regex(@"+\w+$", RegexOptions.Compiled);

    private readonly Regex structuredSyntaxSuffixWildcardRegex = new Regex(@"^\*\+\w+$", RegexOptions.Compiled);

#endif

[kendallb]

Forget compiled regexes. I removed all of them from our code due to massive performance problems similar to you are describing (CPU going nuts). Keeping the regexes static is a good idea, but if you have a ton of compiled regexes I think there is a bug in the .net runtime somewhere as for us it caused the GC to go completely wild and lock up the CPU on our web servers.

[clarity89]

Same thing for me. We've noted 8 calls of Match method with this regex on client creation. That makes high CPU load on concurrent client creations.

[alexeyzimarev]

There is no need to create multiple clients since it is expected to be a singleton, whilst requests are created per call.

[kendallb]

It's good practice to make Regex variables static so they can be re-used as it is fully thread safe.

I doubt very many projects using RestSharp are going to be using a singleton for the client class. I know we certainly do not and almost every library I have seen that uses it creates a client class on demand since it is very cheap to do so.

[alexeyzimarev]

Cheap or not cheap is not always the only consideration. HttpClient is also cheap to create but it is a bad practice to create instances of it on demand and it is preferable to use a pool or a singleton.

I am more than happy to accept a PR though.

[kendallb]

Well it might be bad practice, but it is almost always how people are shown to use it in sample code so it is how most libraries end up using it also, even for HttpClient.

I will make a PR for this tomorrow.

[alexeyzimarev]

I disagree abut the point "it might be a bad practice but this is how people do it". Libraries should not make effort to support bad practices. I am not saying that this is a bad practice for RestSharp right now, before we move to HttpClient, but it might become this.

Looking forward to merge the PR though :)

Concerning HttpClient, this might be of interest https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

[kendallb]

You have some really good points. I was not aware of the issues of using multiple instances of HttpClient, but I am now going to track down all the places we might be using that now and change it. I will do the same for our use of RestClient and consider PR's for libraries we use that don't use a singleton.

What about WebClient? I wonder if it suffers from similar issues or if it is not supposed to be used as a singleton?

[alexeyzimarev]

Tbh I don't know about WebClient, I haven't used it for years.

[kendallb]

In looking at our code, and the fact that RestClient is not disposable, the API surface for the client does not really lend itself well to reuse? Unlike HttpClient, which puts all the important stuff into the requests, it is quite common to use RestClient like this:

    var client = new RestClient {
        BaseUrl = new Uri("https://api.mailgun.net/v3"),
        Authenticator = new HttpBasicAuthenticator("api",  apiKey),
        Timeout = 5000
    };

If you are using it like this, it does not lend itself very well at all to re-use as a singleton instance across multiple threads? It is not clear how you would set a base URL and/or set the authenticator for something like Mailgun, if you planned to use it as a single instance?

[alexeyzimarev]

First, it is not "my code".

Second, the idea behind what I tried to communicate is very simple:

public class MyApiClient : IMyApiClient
{
    private readonly static IRestClient Client = new RestClient("https://myserver/api")
        {
            Authenticator = new HttpBasicAuthenticator("api",  apiKey),
            Timeout = 500
        };

    public async Task<MyResponse> GetMyResponse(MyRequest req)
    {
        var request = new RestRequest(....);
        return Client.ExecuteAsync(request);
    }
}

[kendallb]

Ok so you are suggesting that each API that uses it has a static that is reused, rather than a single static shared across all API calls? Clearly it is not possible to share that.

[alexeyzimarev]

I am not suggesting anything but I cannot see a big issue in delays for the client initialisation since it can be done once for each endpoint. And as I mentioned before, if someone has issues with this and wants to fix it - I am more than happy to accept a PR.

[kendallb]

I will do the PR for the regex since it is still a good fix regardless of wether a single instance is used or not.

[kendallb]

Odd, someone already changed the code to use static regexes. I checked both the develop and master branch. Anyway I found a bunch more static regexes so I did a PR to fix all that. I will submit is now.

[alexeyzimarev]

Fixed by #1120