Locking in js-polykey has gone through alot of iterations. The most recent iteration is in js-db usage of locks where fine grained locks provided by async-mutex is used, as well as the RWLock abstraction in src/utils.ts.
For most domains such as ACL, the locks are too coarse-grained, causing one to lock the entire domain itself. Many of these locks should be replaced with the usage of DB transactions as it is done in js-encryptedfs.
In some places, fine grained locks can replace the existing coarse grained locking or even replace the usage of condition variables. Currently the src/network/Connection.ts and derived classes makes use of a _composed boolean which did double duty in terms of indicating when composition was done but as a way to prevented repeated concurrent calls of compose. The first duty is fine, but the second duty should done with a fine grained lock shared between the all the calls that should be blocked when composition operation is occurring. This means all the methods that currently check _composed and throw exceptions when it is not true.
Transactions has been introduced to js-db. With this we can replace a lot of the existing locking with the use of the new db transactions. The general changes that need to be implemented are as follows.
Updating any transact wrappers by removing them or using withF, withG locking directly.
In all cases where there is a conflict just throw it up the stack. We will expect to handle them within the handlers or look deeper into it later.
ErrorDBTransactionConflict Error should never be seen by the user. We should catch and override it with a more descriptive error for the context.
Transactions should be started within the handlers and passed all the way down to where they are needed. The idea is to make each handler attomic.
Concurrency testing should be introduced but only after SI transactions has be implemented.
All usage of DB should be updated to use the new API. This means removing sublevels and utilising LevelPath and KeyPaths instead.
All usage of db streams should be replaced with the db iterator.
All instances of db.put, db.get and db.del should be using transactions via tran.put/get/del
This applies to all domains that make use of DB OR domains that depend on others that make use of DB. The goal here is to make any even starting from the handlers atomic.
There are limitations to this however. Since a transaction can fail if there is overlapping edits between transactions. We can't really include changes to the db that will commonly or guarantee conflict. Example of this are counters or commonly updated fields. So far this has been seen in;
NotificationsManager. Makes use of a counter so any transactions that include Adding or removing a notification WILL conflict. Reads also update metadata so concurrently reading the same message WILL conflict.
More to follow?
Some cases we will need to make use of locking along with a transaction. A good example of this is in the NotificationManager where we are locking the counter update. When this is the case we need to take extra care with the locking. Unless the lock wraps the whole transaction it is still possible to conflict on the transaction. we can't compose operations that rely on this locking with larger transactions.
An example of this problem is.
start tran1
start tran2
start lock
tran1 update counter
end lock
start lock
tran2 update counter
end lock
end tran1
end tran2 // conflict!
To avoid this tran2 has to start after tran1 ends.
We need to force serialisation of the transactions here.
As a result we can't compose with a transaction outside of
the lock.
This means that some operations or domains can't be composed with larger transactions. It has yet to be seen if this will cause an issue since more testing is required to confirm any problem. I suppose this means we can't mix pessimistic and optimistic transactions. So far it seems it will be a problem with the following domains.
Vaults domain - Lifecycle and editing of vaults relies heavily on locks.
NotificationsManager - Locking is needed for the counter and preventing other conflicts.
Note that this has nothing to do with IPC locking as in #290.
Specification
Locking in js-polykey has gone through alot of iterations. The most recent iteration is in
js-dbusage of locks where fine grained locks provided byasync-mutexis used, as well as theRWLockabstraction insrc/utils.ts.For most domains such as
ACL, the locks are too coarse-grained, causing one to lock the entire domain itself. Many of these locks should be replaced with the usage of DB transactions as it is done injs-encryptedfs.In some places, fine grained locks can replace the existing coarse grained locking or even replace the usage of condition variables. Currently the
src/network/Connection.tsand derived classes makes use of a_composedboolean which did double duty in terms of indicating when composition was done but as a way to prevented repeated concurrent calls ofcompose. The first duty is fine, but the second duty should done with a fine grained lock shared between the all the calls that should be blocked when composition operation is occurring. This means all the methods that currently check_composedand throw exceptions when it is not true.Transactions has been introduced to js-db. With this we can replace a lot of the existing locking with the use of the new db transactions. The general changes that need to be implemented are as follows.
withF,withGlocking directly.ErrorDBTransactionConflictError should never be seen by the user. We should catch and override it with a more descriptive error for the context.LevelPathandKeyPaths instead.db.put,db.getanddb.delshould be using transactions viatran.put/get/delThis applies to all domains that make use of DB OR domains that depend on others that make use of DB. The goal here is to make any even starting from the handlers atomic.
There are limitations to this however. Since a transaction can fail if there is overlapping edits between transactions. We can't really include changes to the db that will commonly or guarantee conflict. Example of this are counters or commonly updated fields. So far this has been seen in;
NotificationsManager. Makes use of a counter so any transactions that include Adding or removing a notification WILL conflict. Reads also update metadata so concurrently reading the same message WILL conflict.Some cases we will need to make use of locking along with a transaction. A good example of this is in the
NotificationManagerwhere we are locking the counter update. When this is the case we need to take extra care with the locking. Unless the lock wraps the whole transaction it is still possible to conflict on the transaction. we can't compose operations that rely on this locking with larger transactions.An example of this problem is.
This means that some operations or domains can't be composed with larger transactions. It has yet to be seen if this will cause an issue since more testing is required to confirm any problem. I suppose this means we can't mix pessimistic and optimistic transactions. So far it seems it will be a problem with the following domains.
Note that this has nothing to do with IPC locking as in #290.
Additional Context
NodeConnectionand network connections and having a race condition withthis._composedbeing true, but the properties that should only be used when composition finishes being undefined.withFandwithGusage in DBTasks
withF,withGlocking directly.ErrorDBTransactionConflictError should never be seen by the user. We should catch and override it with a more descriptive error for the context.LevelPathandKeyPaths instead.db.put,db.getanddb.delshould be using transactions viatran.put/get/del