dmonad / lib0

Monorepo of isomorphic utility functions
MIT License
353 stars 63 forks source link

Mutex Implementation seems incorrect #20

Closed ribx closed 3 years ago

ribx commented 3 years ago

Describe the bug Short Background: I am evaluating at the moment, wether we should switch to yjs from automerge in our software

Looking at the mutex code

  let token = true
  return (f, g) => {
    if (token) {
      token = false
      try {
        f()
      } finally {
        token = true
      }
    } else if (g !== undefined) {
      g()
    }
  }
}```

I am wondering, what that code should accomplish and what it actually does.

The mutex functionality only works, if the mutex is called within the mutex. Something like that:

const mutex = createMutex()

mutex(() => mutex(() => console.log('test')))



Here the mutex prevents the inner code from being executed. But it will not execute the inner function at all and does not inform about it (that is what the `g` function is for i guess, but that is not how I expect a mutex to work and that is also not how it was implemented in yjs). As javascript is single threaded (from a developer point of view) it is not even possible to enter this mutex without nesting a mutex call as shown above.

**Expected behavior**
What i think it should solve: if `f` being an async function, the mutex should not allow other async functions to execute until the running function has ended. See https://github.com/DirtyHairy/async-mutex
dmonad commented 3 years ago

Hi @ribx ,

The documentation above the function explains the functionality:


/**
 * Creates a mutual exclude function with the following property:
 *
 * ```js
 * const mutex = createMutex()
 * mutex(() => {
 *   // This function is immediately executed
 *   mutex(() => {
 *     // This function is not executed, as the mutex is already active.
 *   })
 * })
 * ```
 *
 * @return {mutex} A mutual exclude function
 * @public
 */

This helper function was originally created when async functions didn't exist yet (back in 2014). It was an homage to mutexes that obviously don't need to exist in JavaScript. This "mutex" implementation is helpful when you want to prevent recursion. I.e.

editor.on(delta => {
  mutex(() => {
    ytext.applyDelta(delta)
  })
})

ytext.observe(event => {
  mutex(() => {
    editor.applyDelta(event.delta)
  })
})

This is a typical editor binding. With mutexes, you can prevent recursion. But nowadays I prefer to use the transaction-origin to determine if a change should be ignored.

The function behaves exactly as specified. You don't need to use it. In any case, I won't change the behavior of lib0/mutex because there are several packages depending on it.