0

I have a test automation environment with multithreaded tests that use a shared HttpClient to test methods on our Web API. After the HttpClient has been initialized, it can be used by all of our tests running on multiple threads, since it is a thread safe object. However, keeping the initialization from happening more than once is a challenge. Furthermore, it includes the await keyword in it, so it cannot use any basic lock technology to ensure the initialization operation is atomic.

To make sure the initialization happens properly, I am using a SemaphoreSlim to create a mutex for initialization. To get access to the object, all tests have to call a function that uses the SemaphoreSlim to make sure it has been properly initialized by the first thread to request it.

I found the following implementation for using SemaphoreSlim on this web page.

public class TimedLock
{
    private readonly SemaphoreSlim toLock;

    public TimedLock()
    {
        toLock = new SemaphoreSlim(1, 1);
    }

    public LockReleaser Lock(TimeSpan timeout)
    {
        if (toLock.Wait(timeout))
        {
            return new LockReleaser(toLock);
        }

        throw new TimeoutException();
    }

    public struct LockReleaser : IDisposable
    {
        private readonly SemaphoreSlim toRelease;

        public LockReleaser(SemaphoreSlim toRelease)
        {
            this.toRelease = toRelease;
        }
        public void Dispose()
        {
            toRelease.Release();
        }
    }
}

I use this class like so:

private static HttpClient _client;
private static TimedLock _timedLock = new();

protected async Task<HttpClient> GetClient()
{
    using (_timedLock.Lock(TimeSpan.FromSeconds(600)))
    {
        if (_client != null)
        {
            return _client;
        }

        MyWebApplicationFactory<Startup> factory = new();
        _client = factory.CreateClient();

        Request myRequest = new Request()
        {
            //.....Authentication code
        };

        HttpResponseMessage result = await _client.PostAsJsonAsync("api/accounts/Authenticate", myRequest);
        result.EnsureSuccessStatusCode();
        AuthenticateResponse Response = await result.Content.ReadAsAsync<AuthenticateResponse>();

        _client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", Response.Token);
        return _client;
    }
}

It worked flawlessly until just recently, when I added a ninth thread to my code. I've had to dial that back to 8 threads, because whenever I allow a 9th thread to call the TimedLock.Lock method, the entire program deadlocks.

Does anyone know what might be going on, or how to work around this problem?

John Chesshir
  • 590
  • 5
  • 20
  • Could it be related to the outbound connection limit? https://stackoverflow.com/questions/31735569/what-is-httpclients-default-maximum-connections Try to raise high above 10. – Wiktor Zychla Oct 19 '21 at 18:26
  • IMO this is all overkill. Use `Interlocked.Exchange` and dispose the returned value if it's not null – Charlieface Oct 19 '21 at 23:07
  • 3
    Can you show us how you use the `SemaphoreSlim` in order to protect the initialization of the `HttpClient` object? Also have you tested that your program still deadlocks if you use the `SemaphoreSlim` without the help of the [linked](https://www.rocksolidknowledge.com/articles/locking-and-asyncawait) `TimedLock` class? Also, as a side note, a `Lazy` instance looks like a simpler solution to this problem than using a `SemaphoreSlim`+code-found-on-the-web. – Theodor Zoulias Oct 19 '21 at 23:17
  • @TheodorZoulias Excellent questions. Although I hadn't seen Lazy<> before, I suspect the problem with it will come down to the fact that the initialization code itself has awaits in it, so the same thread cannot be guaranteed to complete the initialization after it has started. I will update the question to include at least pseudo code illustrating that problem. – John Chesshir Oct 20 '21 at 16:12
  • John the `using (_timedLock.Lock(...` is a blocking call, because the `TimedLock` invokes the `Wait` method instead of the correct `WaitAsync`. Which probably results to `ThreadPool` starvation. If your machine has 8 cores, then the `ThreadPool` starts with 8 threads available on demand. Also this line is highly suspicious: `Response = await result...`. What is the `Response`? Does it mean that the `GetClient()` has side-effects? Finally, for asynchronous initialization you can look at [this](https://stackoverflow.com/questions/28340177/enforce-an-async-method-to-be-called-once) question. – Theodor Zoulias Oct 20 '21 at 17:17
  • @TheodorZoulias You're exactly right about the using `(_timeLock.Lock(...` call. See my own answer below, which I finished just about a minute after your comment, and feel free to add your own answer with the details you're talking about. Concerning `Response = await result`, this is an HttpClient construct that allows you not only to wait for the call to finish, but also to wait separately for the full response to finish coming back over the wire. I'll definitely keep `AsyncLazy` in mind, especially if any other problems surface with my current solution. But right now this seems to work. – John Chesshir Oct 20 '21 at 19:11
  • John regarding the `Response`, I meant to ask: where is it defined? I guess that it's a property/field of a class, potentially static. By updating this state from inside the `GetClient()` method, you now have a "side-effecty" method. This is asking for trouble. Why don't you `return` this state from your method, and instead you return the `HttpClient` instance which is already known (stored in the `_client` static field)? – Theodor Zoulias Oct 21 '21 at 03:07
  • @TheodorZoulias Excellent question on the Response. That was one of the member variables that I forgot to include in my snippet. Question is updated with the definition. Regarding why I'm returning the HttpClient instead of the Response: Really the Response was only for evaluation by a particular test of the GetClient() method itself. But it's the HttpClient that is central to all my tests, and must be both instantiated and properly initialized before any tests are allowed to run, so it is not only what gets returned, but it is the subject of the TimedLock object. – John Chesshir Nov 02 '21 at 14:23
  • On second thought, I've just added the type definition to the local function in this question, since having it as a static variable distracts from the topic of this question. – John Chesshir Nov 02 '21 at 15:06

1 Answers1

0

OK. I figured out my own problem, and it really was MY own problem, nobody else's.

If you compare my code above really closely to the source that I quoted as getting it from, you'll notice there's actually a difference. The original code implements the Lock function asynchronously using the WaitAsync function built into SemaphoreSlim:

public async Task<LockReleaser> Lock(TimeSpan timeout)
{
    if(await toLock.WaitAsync(timeout))
    {
        return new LockReleaser(toLock);
    }
    throw new TimeoutException();
}

And of course, in my code that uses it, add the await keyword at the proper place to take care of the added Task object properly:

...
using (await _timedLock.Lock(TimeSpan.FromSeconds(6000)))
{
...

Yesterday I "discovered" that if I changed the toLock object to use WaitAsync, the problem magically went away, and I was SO proud of myself. But then just a few minutes ago, when I was copying and pasting the "original" code into my question, I realized that the "original" code actually included "my" fix!

I now remember looking at this a few months ago and wondering why they needed to make this an Async function. So in my superior wisdom, I tried it without the Async, saw that it worked fine, and continued on until I just recently started using enough threads to demonstrate why it is necessary!

So to keep from confusing people, I changed the code in my question to be the bad code that I originally changed it to be, and put the truly original good code above here in "my" answer, which actually should be credited to Richard Blewett, the author of the referenced article.

I can't say I completely understand why this fix actually works, so any further answers that can explain it better are more than welcome!

John Chesshir
  • 590
  • 5
  • 20