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

Collection was modified; enumeration operation may not execute #398

Closed
Alex2357 opened this issue Nov 29, 2018 · 14 comments · Fixed by #472
Closed

Collection was modified; enumeration operation may not execute #398

Alex2357 opened this issue Nov 29, 2018 · 14 comments · Fixed by #472

Comments

@Alex2357
Copy link

Call failed. Collection was modified; enumeration operation may not execute. GET https://api.com?bla-bla)
----> Flurl.Http.FlurlHttpException : Call failed. Collection was modified; enumeration operation may not execute. GET https://api.com?bla-bla
----> System.InvalidOperationException : Collection was modified; enumeration operation may not execute.

--InvalidOperationException
at System.Collections.Generic.Queue1.Enumerator.MoveNext() at System.Linq.Enumerable.Any[TSource](IEnumerable1 source)
at Flurl.Http.Testing.HttpTest.GetNextResponse()
at Flurl.Http.Testing.FakeHttpMessageHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
at System.Net.Http.HttpMessageInvoker.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
at System.Net.Http.HttpClient.SendAsync(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken)
at Flurl.Http.FlurlRequest.SendAsync(HttpMethod verb, HttpContent content, CancellationToken cancellationToken, HttpCompletionOption completionOption)

@tmenier
Copy link
Owner

tmenier commented Jan 25, 2019

Thanks for the report and the PR. Before I accept it though, I'd like to see a repro, preferably in the form of a test.

@tmenier
Copy link
Owner

tmenier commented Feb 14, 2019

I'm still not clear what specific scenario causes this error. Feel free to re-open if you can provide a repro.

@tmenier tmenier closed this as completed Feb 14, 2019
@tmenier tmenier removed the reviewed label Feb 14, 2019
@Alex2357
Copy link
Author

Alex2357 commented Feb 14, 2019

I thought the error is self explaining. I was using this lib in multiple threads. When 1 thread is removing an item while another was iterating the Queue then this error happens. I can't say for sure what particular operation on Flurl was causing iterating the queue. Actually it is clearly visible from callstack SendAsync is iterating. So the problem: 2 parallel calls to SendAsync cause this problem. The first one removes the item, while another one performs Any(), as result this exception is thrown.

It is not easy to create a test which will reproduce a problem 100%. I had this problem in my test with 30-40% probability. Once I applied that change problem never occurred again.

@tmenier
Copy link
Owner

tmenier commented Feb 14, 2019

Ok great, I'll take another look. I was grooming the backlog and I don't really have a lot of patience for issues that don't have at least a minimal explanation to go with them. But you've provided that now, so all good. Thanks.

@tmenier tmenier reopened this Feb 14, 2019
@tmenier
Copy link
Owner

tmenier commented Feb 14, 2019

I agree with what you did in your PR, but ResponseQueue is public and unfortunately Queue and ConcurrentQueue are not compatible, so it would be a breaking change. I'm close to being ready to start work on 3.0 though, so I'll keep this open and most likely it'll make its way into that.

@tmenier tmenier added this to the Flurl.Http 3.0 milestone Feb 14, 2019
@Alex2357
Copy link
Author

I agree with what you did in your PR, but ResponseQueue is public and unfortunately Queue and ConcurrentQueue are not compatible, so it would be a breaking change. I'm close to being ready to start work on 3.0 though, so I'll keep this open and most likely it'll make its way into that.

I agree this would be a breaking change. One option is to use ConcurrentQueue but the property is just returns snapshot Queue of the ConcurrentQueue

@tmenier
Copy link
Owner

tmenier commented Feb 14, 2019

But if you enqueue something on that snapshot, it wouldn't make it onto the ConcurrentQueue, would it? As I recall, the reason for exposing it at all was to give people a lower-level way to work with the queue when higher-level fluent stuff doesn't provide what you need. I'm good with breaking it in a new major version, but not a minor one.

Also, by sheer coincidence, I just repro'd this bug a few minutes ago while investigating #366. I think better concurrency in the testing framework will be a theme in 3.0.

@Alex2357
Copy link
Author

But if you enqueue something on that snapshot, it wouldn't make it onto the ConcurrentQueue, would it? As I recall, the reason for exposing it at all was to give people a lower-level way to work with the queue when higher-level fluent stuff doesn't provide what you need. I'm good with breaking it in a new major version, but not a minor one.

Also, by sheer coincidence, I just repro'd this bug a few minutes ago while investigating #366. I think better concurrency in the testing framework will be a theme in 3.0.

You right. That would not work.

@tmenier tmenier removed this from the Flurl.Http 3.0 milestone Feb 15, 2019
tmenier added a commit that referenced this issue Feb 15, 2019
@tmenier tmenier removed the next up label Mar 24, 2019
@tmenier tmenier changed the title Collection was modified; enumeration operation may not execute. Concurrency issue with ResponseQueue (use ConcurrentQueue instead?) Apr 26, 2019
@tmenier tmenier changed the title Concurrency issue with ResponseQueue (use ConcurrentQueue instead?) Collection was modified; enumeration operation may not execute Apr 26, 2019
@tmenier
Copy link
Owner

tmenier commented Apr 26, 2019

I'm going to close this issue, because it will be covered by #366. I've mentioned this specific exception there, but for tracking purposes I'd prefer to just keep one of them open.

@karelkral
Copy link

I am experiencing this issue also when trying to load data in parallel.

@tmenier
Copy link
Owner

tmenier commented Dec 6, 2019

This fix will go out in the next 3.0 prerelease.

@jpjohnson
Copy link

@tmenier do you have an estimation on when 3.0 will be released?

@tmenier
Copy link
Owner

tmenier commented Jul 7, 2020

@jpjohnson I don't have an estimate at this time. If the latest prerelease has what you need, I would suggest using it. It's stable.

@jpjohnson
Copy link

@tmenier thank you for your response. I will look at using the prerelease as we have run into this issue.

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

Successfully merging a pull request may close this issue.

4 participants