Fixes a potential race condition in CacheService and reduces memory allocations.

[MarkCiliaVincenti]

No description provided.

[impworks]

I am not convinced that there's an actual problem with the existing code. The external dependency is suspicious, and the whole PR looks automated, so from my point of view the actual purpose for it is either just promoting the package (at best) or paving the path to a supply chain attack (at worst).

[MarkCiliaVincenti]

I have to admit I had a bit of a chuckle reading this :)

Suppose you call GetOrAddAsync with id "123". Let's call this thread A.

So Thread A will create an entry in your ConcurrentDictionary and locks the semaphore. So far so good. It's still processing.
In comes thread B that also tries to make a call with id "123". It finds the entry in the dictionary and obtains the same semaphore being locked by Thread A, so it has to wait on it. So far so good.

At some point Thread A finishes processing. In the finally, it calls the Release method which removes the entry from the dictionary and releases the semaphore. Now thread B is allowed to start processing, and it does.

But now we have an issue. Thread B is processing, but there is no semaphore lock for it in the dictionary. If a thread C comes along and makes a call with id "123" as well, it will find no entry in the dictionary and will start processing concurrently with thread B.

Am I right in assuming your code is trying to specifically avoid this from happening?

[MarkCiliaVincenti]

The library in question is mine. You can call it promoting, even though I earn no money from this. I'm just happy providing a service. AsyncKeyedLock is the most popular locking library, used by Microsoft itself.

As for the part pertaining to reducing memory allocations, that comes from the pooling mechanism within AsyncKeyedLock that allows semaphores to be reused rather than created and disposed every time.

[impworks]

OK, thanks for the clarification - it makes sense now. I admit there was an error in my code, and apologize for my tone and the assumptions I made. However, I still feel that referencing an external library would be an overkill for this case.

I found a better implementation for the Locker type that I used in another project. I thought I copied it here too, but apparently not. Do you see any evident flaws with it (considering that Bonsai is a absolutely not a high-load project)?

  public class Locker<T>
  {
      private readonly Dictionary<T, SemaphoreWrapper> _locks = new Dictionary<T, SemaphoreWrapper>();

      public Task WaitAsync(T id, CancellationToken token)
      {
          SemaphoreWrapper s;
          lock (_locks)
          {
              if (!_locks.TryGetValue(id, out s))
                  s = _locks[id] = new SemaphoreWrapper();
          }

          return s.WaitAsync(token);
      }

      public void Release(T id)
      {
          lock (_locks)
          {
              if (!_locks.TryGetValue(id, out var s))
                  return;

              s.Release();

              if (s.PendingCount == 0)
                  _locks.Remove(id);
          }
      }

      private class SemaphoreWrapper
      {
          public int PendingCount { get; private set; }
          public SemaphoreSlim Semaphore { get; } = new SemaphoreSlim(1, 1);

          public Task WaitAsync(CancellationToken token)
          {
              PendingCount++;
              return Semaphore.WaitAsync(token);
          }

          public void Release()
          {
              Semaphore.Release();
              PendingCount--;
          }
      }
  }

[MarkCiliaVincenti]

I can't see any race conditions but there's no guarantee. If you are 100% sure you don't want to bring in a dependency, I suggest you go with tried and tested.

https://stackoverflow.com/a/31194647

You can compare the above solution with mine in the publicly-available benchmarks that run on Github Actions.

[MarkCiliaVincenti]

Same issues with https://github.com/impworks/isotope/blob/master/src/Isotope/Isotope/Code/Utils/Locker.cs by the way

[impworks]

@MarkCiliaVincenti thanks for your help. I fixed the issue in both projects

[MarkCiliaVincenti]

Aha! Seems I have contributed towards creating more competition 😂

Would you be OK with adding your locker in the benchmarks I'm doing for AsyncKeyedLock?

[impworks]

I doubt that a naive implementation in a library that absolutely nobody uses (except for myself) is much of a competition, but feel free to obliterate me in the benchmark 😆