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

RWLockMap #10

Closed
wants to merge 6 commits into from
Closed

RWLockMap #10

wants to merge 6 commits into from

Conversation

ccorcos
Copy link

@ccorcos ccorcos commented Dec 21, 2023

Hey there!

First off, nice library! I started building this thing myself and quickly found myself very lost in the weeds. It's a wonderful feeling when you grind on something for a bit and then discover an incredibly elegant solution like this in 50 lines of code! My internal dialog said "now this is how you do it!" 😆

Anyways, in its current form, this library works well only for a static number of locks in your program. However, if you're building a database, it is desirable to dynamically create and cleanup locks for things like page-level locks. So that's what this PR implements.

  • First commit implements lock.unlocked property so we know if a lock is currently in use or not.
  • Second commit implements RWLockMap which creates and cleans up locks as needed.

cc @aboodman, @arv

@ccorcos
Copy link
Author

ccorcos commented Dec 21, 2023

Just for the curious, I'm playing around with a generator abstraction that's pretty neat:

export class Locks extends RWLockMap {
	private async multiLock(cmd: LockCmd) {
		const releases = await Promise.all(
			Object.entries(cmd).map(([key, value]) => {
				if (value === "r") return this.read(key)
				if (value === "rw") return this.write(key)
			})
		)

		let called = false
		return () => {
			if (called) return
			called = true
			for (const release of releases) if (release) release()
		}
	}

	async run<T>(fn: () => AsyncGenerator<LockCmd, T, () => void>) {
		const gen = fn()
		let nextValue = await gen.next()
		const releases = new Set<() => void>()
		while (!nextValue.done) {
			const release = await this.multiLock(nextValue.value)
			releases.add(release)
			nextValue = await gen.next(release)
		}
		for (const release of releases) release()
		const result = nextValue.value
		return result
	}
}

This allows you to do stuff like this:

const map = {}

const p1 = locks.run(async function* () {
	yield { a: "r" }
	await sleep(10)
	return map["a"]
})

const p2 = locks.run(async function* () {
	await sleep(2)
	yield { a: "r" }
	return map["a"]
})

const p3 = locks.run(async function* () {
	await sleep(1)
	yield { a: "rw" }
	await sleep(10)
	map["a"] = 1
})

const [r1, r2, r3] = await Promise.all([p1, p2, p3])

assert.deepEqual([r1, r2, r3], [undefined, 1, undefined])

@ccorcos
Copy link
Author

ccorcos commented Jan 3, 2024

Well I'm going to publish my own package in the meantime. If you're interested in this PR, I can fix it up so you don't have that last commit in here. Unfortunately I couldn't just change that from the Github UI - it will have to be a new PR

}

withLock<R>(f: () => R | Promise<R>): Promise<R> {
return run(this.lock(), f);
}

get unlocked() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to call this locked or isLocked.

@@ -40,14 +50,23 @@ export class RWLock {
await Promise.all(this._readP);
const {promise, resolve} = resolver();
this._writeP = promise;
this._readP = [];
return resolve;
this._readP.clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think a Set is needed here. You can also do length = 0 on arrays.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Set will make this._readP.delete(promise); more efficient.

@arv
Copy link
Contributor

arv commented Jan 10, 2024

Thanks for the PR. Sorry about the slow reply. Vacation time over here.

If I understand correctly, the main reason to have RWLockMap is to clear the Lock from the map when it is no longer in use?

We use dynamically created locks stored in a Map in one place (at the top of my head) but we do not bother to clear the Lock when it is no longer in use. The number of entries in the map is pretty small so it never caused us any memory issues.

@ccorcos ccorcos mentioned this pull request Jan 10, 2024
@ccorcos
Copy link
Author

ccorcos commented Jan 10, 2024

I address those changes but I had to create a new PR so its on another branch: #11

@ccorcos ccorcos closed this Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants