RestClientExtensions exposes properties that are not thread safe

[kendallb]

In RestClient we have some public properties that you can modify after the client is constructed, specifically:

    internal Func<string, string> Encode { get; set; } = s => s.UrlEncode();

    internal Func<string, Encoding, string> EncodeQuery { get; set; } = (s, encoding) => s.UrlEncode(encoding)!;

    public IAuthenticator? Authenticator { get; set; }

They can all be access via RestClientExtensions:

public static partial class RestClientExtensions {
    [PublicAPI]
    public static RestResponse<T> Deserialize<T>(this RestClient client, RestResponse response) => client.Deserialize<T>(response.Request!, response);

    /// <summary>
    /// Allows to use a custom way to encode URL parameters
    /// </summary>
    /// <param name="client"></param>
    /// <param name="encoder">A delegate to encode URL parameters</param>
    /// <example>client.UseUrlEncoder(s => HttpUtility.UrlEncode(s));</example>
    /// <returns></returns>
    public static RestClient UseUrlEncoder(this RestClient client, Func<string, string> encoder) => client.With(x => x.Encode = encoder);

    /// <summary>
    /// Allows to use a custom way to encode query parameters
    /// </summary>
    /// <param name="client"></param>
    /// <param name="queryEncoder">A delegate to encode query parameters</param>
    /// <example>client.UseUrlEncoder((s, encoding) => HttpUtility.UrlEncode(s, encoding));</example>
    /// <returns></returns>
    public static RestClient UseQueryEncoder(this RestClient client, Func<string, Encoding, string> queryEncoder)
        => client.With(x => x.EncodeQuery = queryEncoder);

    /// <summary>
    /// Adds cookie to the <seealso cref="HttpClient"/> cookie container.
    /// </summary>
    /// <param name="client"></param>
    /// <param name="name">Cookie name</param>
    /// <param name="value">Cookie value</param>
    /// <param name="path">Cookie path</param>
    /// <param name="domain">Cookie domain, must not be an empty string</param>
    /// <returns></returns>
    public static RestClient AddCookie(this RestClient client, string name, string value, string path, string domain) {
        lock (client.CookieContainer) {
            client.CookieContainer.Add(new Cookie(name, value, path, domain));
        }

        return client;
    }

    public static RestClient UseAuthenticator(this RestClient client, IAuthenticator authenticator)
        => client.With(x => x.Authenticator = authenticator);
}

Unfortunately because RestClient is no longer an instance per request, it is expected to be shared across multiple threads all calling the same API endpoints (single instance per API endpoint). Basically anything configured on the RestClient instance needs to be set up when the instance is created and NOT touched later on. But these are all publicly accessible which indicates to the user they can adjust them for each request,

The Url and Query encoders are a real problem because if two threads change them, you will get unexpected results. The same goes for the Authenticator. If someone changes the Authenticator for a client instance in two different threads, it is going to also cause unexpected results. It would be quite common to use say HttpBasicAuthenticator() to pass in different credentials on each request, but with the current design there is actually no way to do that.

And for cookies, since HttpClient requires the cookie container to be configured on the client, all cookies are going to be a) shared and b) accumulate on every API call, which is really bad. But we can't simply clear the cookie container on every request, because it's a shared resource used by all concurrent requests. Since HttpClient does not support setting a separate cookie container on every request, I suggest the solution is to make the cookies per request, and rendering the cookies into the headers as part of submitting the query as discussed here:

dotnet/runtime#1904

Since RestClient is expected to be used in a single instance fashion, the API surface area for it needs to be changed to reflect that and not allow things to be reconfigured on the fly after an instance is constructed.

So my recommendations are to remove all those properties out of the RestClient implementation and put them into RestRequest, so they can be setup and managed on a per request basis.

[kendallb]

We discussed the issue of single instances way back and you were suggesting at the time to use a single instance per API (which is now what I am working on in our code):

#899 (comment)

In our case the API key for a particular API may not be hard coded in source code at all, but come from configuration. If that API key is changed, since the authenticator is tied to the client, you cannot change that at runtime as you now have a static RestClient instance that is tied to that API. So we would need to either dispose of the client when we detect and API key has changed and create a new one, or I think it makes more sense to move the Authenticator into the request, so it can be changed easily on a per request basis?

[alexeyzimarev]

I think it would be easier to let the authenticator change the token when needed, and wrap it in a lock so it would be thread-safe.

I agree that those properties aren't thread-safe. I am not sure if those need to be properties of the client or part of the options.

As for the cookie container, it might be desirable to have shared cookies for a particular RestClient instance. The most obvious case is authentication cookies. I am not sure how to solve it tbh. Converting cookies to headers could be doable, although I still think that the cookie container should stay. I remember there was an issue for that in this repository.

[alexeyzimarev]

I also remember now that the documentation of the HTTP client factory mentions that when using a custom cookie container, the factory won't work (logically).

[kendallb]

I think it would be easier to let the authenticator change the token when needed, and wrap it in a lock so it would be thread-safe.

How would the authenticator change the token though? Since the authenticator is called on a per request basis, unless the tokens are static it needs to do something to configure them. The standard support with the old way was to set up a new RestClient instance, then set your authenticator. In this case the client is cached. So I think the Authenticator really needs to be part of the RestRequest so it can be configured on a per request basis (as mentioned in the other ticket, but we can continue that discussion here).

I agree that those properties aren't thread-safe. I am not sure if those need to be properties of the client or part of the options.

Since they really should not be changed after the client has been created, I would suggest Encode and EncodeQuery become part of the Options and would not be changed for a client instance. I am not sure if folks would want to change those on a per request basis or not (I would not). But if someone does, you still have a problem. If someone does need to adjust them on a per request basis, then perhaps they need to exist in the RestRequest like the Authenticator does?

As for the cookie container, it might be desirable to have shared cookies for a particular RestClient instance. The most obvious case is authentication cookies. I am not sure how to solve it tbh. Converting cookies to headers could be doable, although I still think that the cookie container should stay. I remember there was an issue for that in this repository.

I am not sure it's desirable to have a shared cookie container for a single RestClient instance it that thing is cached forever. You would get cookie leakage and if the cookies contain user information, you do not want those leaking from one request to another? A shared cookie container is very useful when making back to back requests, but I think the life cycle of that container needs to be defined by the caller. In our case, whenever we need a shared cookie container we set up our own and then configure it on the RestClient, which was always instance per request. We cannot do that now, so the only option is to either stop using the built in cookie container and roll our own, or fix RestSharp so the cookie container becomes part of the RestRequest payload and not the client so we can make sure only the requests we want to share the cookies actually share them?

[alexeyzimarev]

Replied in #1792

[kendallb]

Ok I am working on the changes here. Related to cookies, here is a clear indication that they need to be removed completely from the client:

https://github.com/restsharp/RestSharp/blob/dev/src/RestSharp/RestClientExtensions.Config.cs#L55

        lock (client.CookieContainer) {
            client.CookieContainer.Add(new Cookie(name, value, path, domain));
        }

when we have to lock the cookie container, that indicates other threads will be accessing it. Which means as it is currently working, anyone using cookies is basically cross pollinating cookies across all requests within the same client. Which is really bad. So not only will cookies set in one request end up being submitted to another request, any response cookies will end up in the shared cookie jar from completely unrelated requests.

I am moving the cookies into the request as they are request specific anyway (should never have been on the client). Also as mentioned previously, 99% of folks using RestSharp for REST API's are likely not using cookies. REST API's are almost always stateless and do not rely on cookies.

For the Authenticator, you can see the way JwtAuthenticator is written, it's clearly wrong and should also not be part of the client at all:

https://github.com/restsharp/RestSharp/blob/dev/src/RestSharp/Authenticators/JwtAuthenticator.cs#L24

    [PublicAPI]
    public void SetBearerToken(string accessToken) => Token = GetToken(accessToken);

That is a public API function on the authenticator to change the bearer token for the authenticator after it has been created and assigned to the client. Clearly that was done so that the token can be changed after it was created, but if this authenticator is attached to a single instance client for an API, if there are valid reasons to need to change this on a request basis, you will have serious problems because multiple threads will potentially see the wrong bearer token.

I am moving the Authenticator into the request and while it's feasible to allow it also on the client and have the request one be an override, I would advocate to have it only on the request. After all request authentication is, well, request centric. It has nothing to do with the client setup and I think it should never have been on the client in the first place. It's another code smell from the client API design from the past that told everyone using it, you need a new instance for each request.

Hence while this would be a breaking change, I believe it's actually quite important to cause this breakage to make sure anyone currently using v107 with single instance clients is required to adapt their code as it's currently potentially wrong if they are trying to set or change the authenticator on a per request basis.

[kendallb]

This will be fixed by:

#1795

[stale]

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.

[repo-ranger]

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

[stale]

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.