Skip to content
This repository has been archived by the owner on Jun 10, 2019. It is now read-only.

refactor(state): locks and concurrency #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mvayngrib
Copy link
Contributor

@mvayngrib mvayngrib commented Mar 1, 2019

for issue #17

let me know what you think

i factored out the locking/queueing logic into a separate module: multi-lock-queue, feel free to copy it over if you like. Actually, I coded it up a while back, when I first saw your issue and got curious about the problem. The code is short, should be pretty simple to follow, and there are tests. I took a look at async-lock, since you're already using it, but when I peeked inside the code, I got lost

the resulting changes of integrating the module are pretty straightforward. There's a queue that supports locks, and pausing it is the equivalent of waiting to acquire a global lock.

There were some unintended changes in the package-lock.json, no matter how many times I rimraf'd and re-installed, idk how you want to go about that. What version of npm are you using?

@smartcontracts
Copy link
Contributor

So generally I'm a fan of the "standard" locking API of lock.acquire() and lock.release(). I know we started with the locking queue mechanism but may be easier to rewrite our existing stuff to do this by acquiring a lock once and doing everything synchronously or, if it's an operation that can happen async, have each "thread" acquire the lock for itself.

@smartcontracts
Copy link
Contributor

smartcontracts commented Mar 1, 2019

As a general rule of thumb I like the idea of writing libraries for ourselves (for maintainability and security), so not relying on async-lock makes sense 👍

@mvayngrib
Copy link
Contributor Author

mvayngrib commented Mar 1, 2019

@kfichter the problem with acquiring & releasing locks is you now have to do three jobs everywhere: lock, run something, and then release no matter what happens. Isn't it easier for the try/catch/finally to be abstracted away by enqueue()?

@smartcontracts
Copy link
Contributor

Hmmm... that's true. It's part of why I like the syntax of async-lock - it's very similar to the with lock ... syntax in python that automatically releases the lock after execution.

@mvayngrib
Copy link
Contributor Author

maybe we can just change the name/signature of enqueue. What would you prefer the invocation to look like? Something like await this._locks.withLocks(fn, locksArr) ? You tell me :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants