Please don't deprecate release() function

[Aryk]

With great power comes great responsibility...says Batman I think ;)

I get it, runExclusive ensures you don't screw things up and not unlock the thread, but sometimes you can't set things up with a nice try / catch loop and you really need to use withTimeout to ensure it doesn't lock the application.

Here is an example from React Native that I made as a workaround for this Expo issue:

import React, {useCallback, useRef, useState} from "react";
import {MutexInterface, withTimeout, Mutex} from "async-mutex";
import {Video} from "expo-av";
import {Asset} from "expo-media-library";

type IVideoInfo = Pick<Asset, "width" | "height" | "duration">;

const useVideoInfo = () => {
  const [uri, setUri] = useState<string>();
  // const mutexRef      = useRef<MutexInterface>(withTimeout(new Mutex(), 4000));
  // withTimeout didn't really seem to be working...so we just set a timeout
  const mutexRef      = useRef<MutexInterface>(new Mutex());
  const videoInfoRef  = useRef<IVideoInfo>();

  const getVideoInfo = useCallback(
    async (uri: string) => {
      mutexRef.current.release();
      videoInfoRef.current = undefined;

      await mutexRef.current.acquire();
      setTimeout(() => mutexRef.current.release(), 5000);
      setUri(uri);

      await mutexRef.current.waitForUnlock();
      if (!videoInfoRef.current) {
        throw(`Could not extract the video info from ${uri}`)
      }
      return videoInfoRef.current;
    },
    [],
  );

  const VideoInfoExtractor = useCallback(
    () => <Video
      style={{width: 1, height: 1, opacity: 1}}
      source={{ uri }}
      onReadyForDisplay={event => {
        if (event.status?.isLoaded) {
          const { width, height } = event.naturalSize;
          videoInfoRef.current = {width, height, duration: event.status.durationMillis / 1000};
          mutexRef.current.release();
        }
      }}
    />,
    [uri],
  );

  return {
    VideoInfoExtractor,
    getVideoInfo,
  };
};

export {
  useVideoInfo,
  IVideoInfo,
};

Rewriting this somehow with runExclusive would basically fly in the face of what I'm trying to do here which is wait for one thing to wait for another.

Can we keep the acquire/release paradigm into the future? 🙏

PS. For some reason the withTimeout wrapper didn't work for me...so I had to use setTimeout...maybe I'm doing something wrong?

[cscheid]

+1 please please don't deprecate release :)

release (to use, um, boomer language, V) before acquire (P) on a semaphore initialized to zero can be used to get (effectively) condition variables, which is pretty nice to have since async-mutex doesn't implement those directly.

[cscheid]

Actually, I think in order for what I'm suggesting to work, the library needs to drop the (over-zealous, I'd argue) error check in the Semaphore constructor. There are uses for initializing a semaphore's value to zero (and even negative values potentially).

[DirtyHairy]

It continue to be surprised by the way this library is used in projects 😏

A bit of background on why I have been planning to remove Mutex::release (the same holds true for Semaphore). This library originally was intended for limiting concurrency in critical sections of code. To that end, the original API did not contain Mutex::release; instead, Mutex::acquire returns a dedicated release callback that is scoped to the individual lock acquired by acquire. This has the benefit of being idempotent: multiple calls will have no effect, even if the mutex has been locked again. In order to have a RAII type approach to unlocking, I also added runExclusive from the start.

I only added Mutex::release when people started requesting it in order to avoid managing the individual releasers returned by subsequent calls to acquire. When I added it I was in a hurry, and I only later realized that this use case is much better served by runExclusive. So, I decided to deprecate and remove it again.

However, judging from your example (and from other requests for keeping this function) it seems that people are now also using the library now to notify waiters when a task has completed, and this cannot be refactored to use runExclusive (and scoped releasers are awkward in this case). I guess I can't really go back and remove that function again. So, worry not, I'll keep it (and probably also make it work properly for nontrivial semaphores) 😏 I'll remove the annotation and adapt the documentation when I next update the library.

As for using the semaphore as a condition variable: I would much rather add an async queue to the library. You would call await queue.next() to wait for the next value to be pushed into the queue, and calling queue.push(...) would push the next value into the queue. New values would be distributed to waiters in a first come, first served fashion. I think this would probably cover all use cases that I have seen where mutex / semaphore are used for notification rather than for limiting concurrency. Would that fit your shoe?

[cscheid]

First of all, thank you so much for responding and for the library <3.

To answer your question, I think the answer is no, at least not without either a wrapper class (that would effectively be a condition variable) or some client logic at every call site (which is a risky thing with concurrent programming..)

Our specific use case is the initialization of some shared state which we need to do on demand. The calls that cause the initialization are async and we can't control their order (or whether they happen at all). On top of it, the initialization procedure itself is also async and takes potentially long. The cleanest solution I found is a critical section (call it "mustInit") wrapping a 0-initialized semaphore (call it "hasInit"). After the state has been properly initialized, the semaphore is released (to 1). Everywhere else, the semaphore is used only as a "wait". (that is, as semaphore.runExclusive(async () => {}))

If you're interested, the code is here and here.

In the end, I ended up rolling our own version of a semaphore (because I needed the 0-init case as well) which is here.

[Aryk]

However, judging from your example (and from other requests for keeping this function) it seems that people are now also using the library now to notify waiters when a task has completed, and this cannot be refactored to use runExclusive (and scoped releasers are awkward in this case). I guess I can't really go back and remove that function again. So, worry not, I'll keep it (and probably also make it work properly for nontrivial semaphores) 😏 I'll remove the annotation and adapt the documentation when I next update the library.

Awesome 🙌

[DirtyHairy]

If you're interested, the code is here and here.

Thanks alot for the example, interesting. I guess my own approach here would have been using a simple Promise that resolves once initAutomation completes. Every consumer would then just await said promise. However, there's always more than one way to skin a cat, and I guess there are other implications from this that I don't see by skimming the code.

I can see there are other, similar scenarios where one piece of code wants to wait on a signal from another piece of code that are not easily covered by working with plain promises (or events, which is another approach that I usually take in such situations), but that are served better by a generalized mutex or semaphore.

After dwelling a bit more on this I think I will extend the Semaphore implementation to allow arbitrary initial and max values (including uncapped semaphores). This will take some refactoring to make sure no part of the current API breaks in subtle ways though.

[DirtyHairy]

Version 0.4.0 removes the deprecation.