HttpTest.ResponseQueue is not thread safe; replace Queue with ConcurrentQueue

[glen-nu]

[Edit by @tmenier]
In highly concurrent test scenarios, equeuing/dequeuing simulated responses can lead to one or both of these errors:

1. "Collection was modified" exception (reported in Collection was modified; enumeration operation may not execute #398)
2. Same response gets dequeued multiple times (reported below)

Ideal fix for both is to use ConcurrentQueue instead of Queue to implement HttpTest.ResponseQueue. But unfortunately, since these do not implement a common interface, this will be a breaking change. So it will need to wait until 3.0.

---

[original bug report]
I'm trying to perform 2 requests simultaneously a little like so (.NET Core 2.0):

var task1 = Request1();
var task2 = Request2();

var result1 = await task1;
var result2 = await task2;

This works fine in production, but when I attempt to test it with HttpTest, I occasionally run into the error Cannot access a closed Stream. Digging a little deeper, it seems that multiple threads are attempting to dequeue from the response queue and in some cases, the same response can be returned multiple times. It might be solvable with a thread-safe queue.

[tmenier]

Could you provide a minimal example of what the test looks like where this is happening?

[glen-nu]

Sure, this will be highly simplified, I hope it's accurate. This is in the context of an XUnit controller test that performs some http calls:

// controllerTest.cs
public async Task HttpTestBehavesStrangelyHere() {
	_httpTest.RespondWith(a);
	_httpTest.RespondWith(b);
    HitControllerEndpoint();
}

// controller.cs
// FlurlClientFactory has already been injected via DI

public async Task PerformTwoHttpRequests() {
	// Here I expect task1 and task2 to reliably receive a then b
	// because of the responses have been pushed into the queue.

	// In reality this appears that multiple threads attempt to pop from
	// the response queue and Flurl sometimes tries to send the same response back   
	var task1 = Client.Request(a).GetJsonAsync<Something>();
	var task2 = Client.Request(b).GetJsonAsync<SomethingElse>();

	// we want to await these two requests in parallel
	var result1 = await task1;
	var result2 = await task2;
	// ....
}

[tmenier]

Sorry for the delayed reply. This sounds legit, I'll look into it for the next release. One thing to note is that when you fire off multiple tasks concurrently like this, I'm not certain that getting the responses back in order will be guaranteed. But if it's returning the same fake response for multiple requests, I agree that's a bug.

[glen-nu]

Yeah I think I'm getting some combination of both scenarios, but it's pretty difficult to replicate the scenarios. Thanks!

[tmenier]

I am not able to repro this. Here is my test, similar to yours but taken to extremes:

[Test]
public async Task can_use_response_queue_in_parallel() {
    var cli = new FlurlClient("http://api.com");

    for (var i = 0; i < 10000; i++) {
        using (var test = new HttpTest()) {
            test
                .RespondWith("0")
                .RespondWith("1")
                .RespondWith("2")
                .RespondWith("3")
                .RespondWith("4")
                .RespondWith("5")
                .RespondWith("6")
                .RespondWith("7")
                .RespondWith("8")
                .RespondWith("9")
                .RespondWith("10")
                .RespondWith("11")
                .RespondWith("12")
                .RespondWith("13")
                .RespondWith("14")
                .RespondWith("15")
                .RespondWith("16")
                .RespondWith("17")
                .RespondWith("18")
                .RespondWith("19");

            var results = await Task.WhenAll(
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync(),
                cli.Request().GetStringAsync());

            CollectionAssert.IsEmpty(test.ResponseQueue);
            CollectionAssert.AllItemsAreUnique(results);
        }
    }
}

This is basically testing 20 calls in parallel, asserting that the same response was never returned twice and that the response queue is empty, and just be sure it repeats that test 10,000 times. For me, it's passing on .NET Core and full framework.

I wonder if something else is going on in your case. You don't have static instance of HttpTest do you?

[glen-nu]

We're using xUnit 2.3.1. In the test class constructor we instantiate a new HttpTest instance and that gets used by each test (we dispose of each instance after each test).

I think you might be on to something though; we can finally reproduce the bug and it seems that we only encounter the issue when we run multiple tests in the same run, even though the tests don't run in parallel. We'll investigate a little more, but do let me know if you think HttpTest isn't being used correctly here.

[zhiwei-nu]

@tmenier I was able to repro with the snippet below.

TL;DR: the flurl client needs to do some file IO on BeforeCallAsync. It seems that the first mocked response gets returned for all responses.

        [Fact]
        public async Task can_use_response_queue_in_parallel()
        {
            var cli = new FlurlClient("http://api.com");
            cli.Configure(settings =>
            {
                settings.BeforeCallAsync = async call =>
                {
                    // Comment all these and res2 will get "1" as response
                   // somefile.json is a very small json with 10
                    await File.ReadAllTextAsync("somefile.json");
                    await File.ReadAllTextAsync("somefile.json");
                    await File.ReadAllTextAsync("somefile.json");
                };
            });

            using (var test = new HttpTest())
            {
                test.RespondWith("0");
                test.RespondWith("1");

                var task1 = cli.Request().GetStringAsync();
                var task2 = cli.Request().GetStringAsync();
                var res1 = await task1;
                var res2 = await task2;

                Assert.Empty(test.ResponseQueue);
                Assert.Equal("0", res1);
                Assert.Equal("1", res2);
            }
        }

# somefile.json
{
  "Never": "Gonna",
  "Give": "You",
  "Up": "!",
  "Never": "Gonna",
  "Let": "You",
  "Down": "!"
}

It seems some file IO is needed to reproduce this, and just waiting with System.Threading.Thread.Sleep(1000) won't work. Not entirely sure what's happening here but hopefully you'll have a better shot than me at this :)

[tmenier]

Ahhh, yep, all it took for me was introducing an artificial delay in BeforeCallAsync and I encountered a couple different errors.

cli.Settings.BeforeCallAsync = call => Task.Delay(1000);

I'll keep investigating.

[tmenier]

Got a community contrib to back the response queue with a ConcurrentQueue, which works great, but then my test above revealed that CallLog needs to be thread-safe too. So that's also backed by a ConcurrentQueue but exposed as IReadOnlyList. This will go out with the next 3.0 prerelease.

[tmenier]

#482 had major implications on the design of the queue with respect to thread safety. It's basically not a queue anymore at all but list with an indexer that gets incremented in a thread-safe way. Building up the responses is not thread-safe, but I don't think it needs to be. "Dequeuing" is though, and I think that's the important part. My test is passing, but the critical line of code is here if anyone wants to give it the eyeball test for thread safety.

[tmenier]

Now available to test: https://www.nuget.org/packages/Flurl.Http/3.0.0-pre2