FlurlRequest.WithCookie throws System.InvalidOperationException: Collection was modified; enumeration operation may not execute

[rcollette]

I create a flurl client

 var client = new FlurlClient("http://url");

I have the following method that I call where the single instance of that client is passed in.

        private static async Task<Result> GetAndCheck(Credentials credentials, LoginService loginService,
            IFlurlClient client)
        {
            try
            {
                var authToken = await loginService.getAuthToken(credentials);
                if (string.IsNullOrEmpty(authToken))
                {
                    Console.WriteLine($"null authtoken for user {credentials.username}");
                    return null;
                }
               var result = await client.Request()
                    .WithBasicAuth("user", "password")
                    .WithHeader("cache-control", "no-cache")
                    .WithCookie("authToken", authToken).GetAsync();
...
...

Being called via

var tasks = ids.Select(creds => GetAndCheck(creds, loginService, client));

So about 200 tasks being created simultaneously, each making a request to get an authToken on one Flurl client instance (in loginService) and then making another request with a different Flurl client instance (client).

I am periodically getting the exception.

Flurl.Http.FlurlHttpException: Call failed. Collection was modified; enumeration operation may not execute. GET http://localhost:8081/api/v1/users/context ---> System.InvalidOperationException: Collection was modified; enumeration operation may not execute.
   at System.ThrowHelper.ThrowInvalidOperationException_InvalidOperation_EnumFailedVersion()
   at System.Collections.Generic.Dictionary`2.ValueCollection.Enumerator.MoveNext()
   at Flurl.Http.FlurlRequest.WriteRequestCookies(HttpRequestMessage request)
   at Flurl.Http.FlurlRequest.<SendAsync>d__19.MoveNext()
   --- End of inner exception stack trace ---
   at Flurl.Http.FlurlRequest.<HandleExceptionAsync>d__24.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at Flurl.Http.FlurlRequest.<SendAsync>d__19.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Flurl.Http.FlurlRequest.<SendAsync>d__19.MoveNext()

But the definition for the Cookies property in FlurlRequest is

    public IDictionary<string, Cookie> Cookies
    {
      get
      {
        return this.Client.Cookies;
      }
    }

So when I use WithCookie on the request, which references the Cookies property of the request, it is going to modify the single shared client. Likewise the WriteRequestCookies method does a Merge on FlurlRequest.Cookies with this.Client.Cookies as the source, but this is then merging the same reference into itself.

This is not something that can be fixed with a custom client factory like:

class NoCookieTrackingClientFactory : DefaultHttpClientFactory
    {
        public override HttpMessageHandler CreateMessageHandler()
        {
            return new HttpClientHandler {UseCookies = false};
        }
    }

or using new FlurlClient(...).Configure(settings=> settings.CookiesEnabled=false) because the WithCookie extension is still referencing FlurlRequest.Cookies which is actually the shared Client instance.

At the moment it would seem the only thing I can do when trying to send a cookie with a request is to create a new FlurlClient each time.

[tmenier]

Yeah, this is a known issue. Cookies were a _big_ hurdle in moving all stateful things to the request level so FlurlClient could be heavily reused in 2.0. I ended up punting it to (tentatively) the next major release. I know it's a bit unintuitive that you can set cookies at the request level but they get written to the client, but for now just know that that's how it works and sharing cookie-enabled clients with different cookies on different threads will lead to problems. I recommend going with client per cookie "session" pattern.

[rcollette]

Sorry for coming back to this after so much time. Perhaps something to consider would be to allow the specification of a CookieManager. In my particular case I would implement it such that cookies that are saved/set would simply be dumped (no-op). Other cross-platform systems like QT rely on an application/platform dependent implementation of a cookie store.

I imagine this could be done by taking your current cookie implementation, moving it to a DefaultCookieManager but allowing the client's CookieManager default to be overwritten. It should be possible to add the flexibility while not breaking existing code.

[IGx89]

Just ran into this issue myself. This is a pretty big issue given that it's not obvious in the documentation that this behavior is present, meaning if someone follows the docs about passing cookies their code is guaranteed to have this concurrency issue. Even worse from a security standpoint, it could intermix different sessions' cookies.

[tmenier]

I'm actively gathering feedback to help prioritize issues for 3.0. If this one is important to you, please vote for it here!

[tmenier]

I've finally started to tackle per-request cookies and I'm hitting some new hurdles. The existing mechanism piggy-backs on HttpClientHandler's CookieContainer functionality, which is handy for automatically plucking cookies from the response headers and adding them to the next request. Of course it falls apart when you're trying to do multiple "sessions" with the same FlurlClient. So, I experimented with setting HttpClientHandler.UseCookies to false and parsing the headers myself. Fairly straightforward, until one my tests failed. It uses httpbin's cookie/set endpoint, and the reason it failed is because that endpoint does a redirect after sending the cookie response headers (probably typical in the real world), HttpClientHandler.AllowAutoRedirect is enabled by default, and there's no way to access the original response headers before the redirect. So, I'd have to disable AllowAutoRedirect and reinvent that wheel too. Ugh.

I've opened a suggestion with .NET team, but I have no intention of making 3.0 wait on that. I think I have 2 options:

1. Move forward with the wheel reinventing.

2. Live with the client-per-cookie-session constraint, but design a better API, i.e. make it easy to create a new "cookie session", and don't make it look like it's per-request when in fact it isn't.

Thoughts? Obviously #2 is easier from my end. And by most accounts, socket exhaustion doesn't really become a problem until your client instances reach several thousand.

[tmenier]

I've moved forward with wheel-reinventing:

506

500

I'd like to gather as much feedback as possible, especially in #506 from anyone who needs to interact with cookies. In any event, that issue should resolve this one.

[tmenier]

I'm not likely to fix this in 2.x, and I believe it becomes irrelevant in 3.0 due to the overhaul in #506. Anyone opposed to closing this issue at this point?

[rcollette]

I reread the entire 3.0 implementation notes. Looks good to me. Good to close.

[tmenier]

@rcollette Sounds good. I'm hoping you have a chance to test prerelease 4. I should point out that I didn't make a great effort to ensure thread safety with CookieJar; it is backed by a ConcurrentDictionary but its custom methods shouldn't be considered atomic. But the whole point is that it allows a usage pattern of many CookieJars (one per "user session") with a singleton client, which wasn't possible before with CookieContainer.

In other words, go ahead and try to break it (it needs that!), but keep the recommended usage pattern in mind too. :)