Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object map locks should be removed in favour of transaction locks #443

Closed
CMCDragonkai opened this issue Aug 23, 2022 · 10 comments
Closed
Assignees
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@CMCDragonkai
Copy link
Member

Specification

We should remove all the object map locks in favour of the transaction locks. This is because locks don't compose, and all the object maps are just representations of the source of truth on disk. Ideally we should use the tran.queueSuccess and such that can do in-memory changes only after a commit occurs. This means local state inside a transactional function is temporary, and only after a commit succeeds, does global (domain state) get mutated.

Generally I reckon we shouldn't have any operations that is mutating the in-memory state without also involving the underlying disk, so there shouldn't be a situation where they can't coordinate with the transaction locks.

The transaction locks are far more useful generally because they are re-entrant, and they don't release until transaction destruction.

Originally posted by @CMCDragonkai in #419 (comment)

Additional context

  • Integrate js-db and concurrency testing #419 - discovered issues with the vault map locks and the transaction locks, transaction locks are re-entrant, vault map locks are not, also vault map mutation can be done with the queueFinally.

Tasks

  1. Identify all object map lock usage
  2. Prototype the change to using transaction locks with global domain state mutation done by queueFinally
  3. Make the change, and test concurrent behaviour
@CMCDragonkai CMCDragonkai added the development Standard development label Aug 23, 2022
@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Aug 23, 2022

The #382 should be done first before this so we can have proper verification of our concurrent behaviour now.

Removing the object map locks means we should still verify whether our mutation doesn't have any race conditions during in-memory object creation.

@CMCDragonkai
Copy link
Member Author

@tegefaulkes what object map locks do we still have?

  1. Nodes?
  2. Vaults?
  3. Any where else?

@tegefaulkes
Copy link
Contributor

I think those are the only two.

@CMCDragonkai
Copy link
Member Author

The proxy had their object map locks replaced recently. So there were 3.

Nodes and vaults still has it.

@tegefaulkes can you also address the nodes change in your next PR.

@tegefaulkes
Copy link
Contributor

The nodes domain object maps were already replaced with LockBox. So i think that's addressed.

@CMCDragonkai
Copy link
Member Author

Great so only vaults left.

@CMCDragonkai
Copy link
Member Author

So vaults domain is the only place where it's actually replacing object map locks with transaction locks. The other 2 are just using in-memory lockbox.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jul 10, 2023

@tegefaulkes is the vaults domain still using object map locks or has it been transitioned to LockBox?

If it has been transitioned, this should be closed.

@tegefaulkes
Copy link
Contributor

I think vaults was updated with c256620.

There may be an object map in the Proxy before that is removed fully in agent migration.

@CMCDragonkai
Copy link
Member Author

Ok since that is to be removed, then this is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity
Development

No branches or pull requests

2 participants