feat: Add a Redis based Read-Write Locker #1263

Falx · 2022-04-13T13:53:34Z

📁 Related issues

✍️ Description

This PR introduces a Redis-based Locker that implements the Read Write locking algorithm. This implementation abides to the following rules:

Simultaneous reads are allowed as long as no write lock is present
Only one exclusive write lock is allowed as long as no other write lock AND no other reads are present
When multiple processes are waiting to acquire a locked resource, there is no guaranteed order in which they can acquire the lock once the previous lock is released.
The implementation should be thread safe because of the following reasons:
- No references to the lock are exposed other than a string identifying the key in redis (which is based on the identifier path)
- Redis executes its commands single threaded
- Redis custom commands (implemented as lua scripts) are therefore considered atomic.
- 4 custom commands where implemented with lua scripts (acquireReadLock, acquireWriteLock, releaseReadLock, releaseWriteLock)

Files

name	path	description
RedisReadWriteLocker.ts	`src/util/locking`	The implementing class
RedisLuaScripts.ts	`src/util/locking/scripts`	Contains actual lua scripts, extra typing and some helper conversion functions
redis-rw.json	`config/util/resource-locker`	Componentjs config
RedisReadWriteLocker.test.ts	`test/unit/util/locking`	Unit tests
RedisReadWriteLockerIntegration.test.ts	`test/integration`	Integration test
run-with-rw-redis-lock.json	`test/integration/config`	Config for integration test

Remarks
I am not really sure about how to fit this into the WrappedExpiringReadWriteLocker. Right now the RedisReadWriteLocker is implemented with non-expiring keys (I can configure the lua scripts to make the keys auto-expire if needed, but they don't right now). I was under the impression that this was preferrable to allow for an external way to expire keys (e.g. with that WrappedExpiringReadWriteLocker). I might need to do more changes to support this though.

I had to update the ioredis package, so that is way I made some small changes to the existing RedisResourceLocker class.

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

this PR is labeled with the correct semver label
- semver.patch: Backwards compatible bug fixes.
- semver.minor: Backwards compatible feature additions.
- semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
the RELEASE_NOTES.md document in case of relevant feature or config changes.
any relevant documentation was updated to reflect the changes in this PR.

Falx · 2022-04-13T14:17:15Z

Locally after setting the environment variable TEST_DOCKER to true, the integration test runs successfully on my pc. (I start a separate docker instance with Redis on port 6379)

joachimvh

I am not really sure about how to fit this into the WrappedExpiringReadWriteLocker.

Not sure what you mean. The WrappedExpiringReadWriteLocker takes as input a ReadWriteLocker so this should work fine with this new locker right?

I was under the impression that this was preferrable to allow for an external way to expire keys (e.g. with that WrappedExpiringReadWriteLocker).

That's fine for me.

As an aside, how much effort would it be to make the RedisResourceLocker thread safe using the same method as is used here? Might it perhaps be possible to even merge these two into a single class that implements both ReadWriteLocker and ResourceLocker? Because it feels a bit much now that we have 2 different redis locker classes that are quite similar.

config/util/resource-locker/redis-rw.json

config/util/resource-locker/redis.json

package.json

src/util/locking/RedisReadWriteLocker.ts

src/util/locking/scripts/RedisLuaScripts.ts

test/util/Util.ts

Falx · 2022-04-14T08:26:40Z

As an aside, how much effort would it be to make the RedisResourceLocker thread safe using the same method as is used here? Might it perhaps be possible to even merge these two into a single class that implements both ReadWriteLocker and ResourceLocker? Because it feels a bit much now that we have 2 different redis locker classes that are quite similar.

To be honest, the way RedisResourceLocker works, by using the redlock library and saving the returned Lock object in memory, it is hard to make it multi-process-safe. What I can do however is adapt my solution to also implement ResourceLocker. This has some implications:

I would not include redlock, but implement it purely with ioredis (like I did now, but it would be a bit more simple).
The current RedisResourceLocker would become obsolete in my opinion.

joachimvh · 2022-04-14T09:01:38Z

To be honest, the way RedisResourceLocker works, by using the redlock library and saving the returned Lock object in memory, it is hard to make it multi-process-safe.

Yes I meant removing the Redlock references and using Lua like you already do in the new solution. Which comes down to your suggestion of adapting your solution.

Falx · 2022-04-15T08:47:20Z

@joachimvh Do you have any idea why the integration (linux) tests are not working on here? I can execute them in a WSL2 (ubuntu) shell with a redis docker service on my pc..

joachimvh · 2022-04-15T08:55:32Z

@joachimvh Do you have any idea why the integration (linux) tests are not working on here? I can execute them in a WSL2 (ubuntu) shell with a redis docker service on my pc..

I have no idea tbh. Might be an issue with the redis instance set up by the github actions in that the latest ioredis version doesn't work nicely together with it? I would suggest creating a temporary commit with extra console.log debug statements or something else that can help figure out the issue with the CI.

It also seems that your new integration tests succeed and only the previous class fails, so maybe a Redlock case? But that class can be removed once RedisReadWriteLocker is both a ReadWriteLocker and a ResourceLocker since there is no point in having 2 redis resource lockers, which I think is already the case looking at the commits? So perhaps just removing that class and its corresponding test solves the issue already.

Would also rename the class to RedisLocker seeing as it is 2 kinds of lockers at the same time.

Falx · 2022-04-15T08:58:02Z

It also seems that your new integration tests succeed and only the previous class fails, so maybe a Redlock case? But that class can be removed once RedisReadWriteLocker is both a ReadWriteLocker and a ResourceLocker since there is no point in having 2 redis resource lockers, which I think is already the case looking at the commits? So perhaps just removing that class and its corresponding test solves the issue already.

Would also rename the class to RedisLocker seeing as it is 2 kinds of lockers at the same time.

Ok, that was my next plan. Getting the old one out and replacing with a RedisLocker. I will squash everything in one commit then and see if the intergration test is fixed.

Falx · 2022-04-15T16:04:56Z

I've merged the 2 Redis integration tests into 1. I also added a section that tests the lua scripts themselves.

joachimvh

Looking good, I like the improvements. All comments are mostly minor/styling issues.

.vscode/settings.json

config/util/resource-locker/redis-rw.json

config/util/resource-locker/redis.json

src/index.ts

src/util/locking/RedisLocker.ts

src/util/locking/scripts/RedisLuaScripts.ts

refactor: more elegant way of providing default attemptSettings to constructor style(jsdoc): rewording of jsdoc comment fix: RegExp(/regex/) => /regex/ fix: Replace Error with InternalServerError docs: jsdoc for RedisReadWriteLocker class feat: make RedisReadWriteLocker a ResourceLocker too test: coverage back to 100% refactor: linting fix style(jsdoc): Add explanation to tryRedisFn() method refactor: remove RedisResourceLocker fix: bug in lua script chore(deps): update ioredis, remove redlock refactor: removed RedisResourceLocker in favor of generic RedisLocker class test: add redis lua scripts tests and integrate all 3 redis integration tests in 1 refactor: remove .vscode folder from index refactor: Add some typing and change redis references to Redis in comments refactor: more changes after PR review refactor: remove redis.json refactor: rename redis-rw.json to redis.json docs: added readme and release notes

joachimvh

Looks good, thanks!

RubenVerborgh requested a review from joachimvh April 13, 2022 13:54

Falx marked this pull request as ready for review April 13, 2022 13:58

joachimvh requested changes Apr 14, 2022

View reviewed changes

Falx force-pushed the feat/redis-read-write-lock branch 2 times, most recently from 1bd56c8 to b2a595b Compare April 15, 2022 16:03

Falx force-pushed the feat/redis-read-write-lock branch 5 times, most recently from b8430da to 121d612 Compare April 16, 2022 12:04

Falx requested a review from joachimvh April 16, 2022 12:13

joachimvh requested changes Apr 19, 2022

View reviewed changes

Falx force-pushed the feat/redis-read-write-lock branch from b68b45f to c937b96 Compare April 19, 2022 10:21

Falx requested a review from joachimvh April 19, 2022 11:25

Falx force-pushed the feat/redis-read-write-lock branch from c937b96 to 16631ab Compare April 19, 2022 11:40

joachimvh approved these changes Apr 19, 2022

View reviewed changes

joachimvh merged commit e2e2d08 into versions/4.0.0 Apr 19, 2022

joachimvh deleted the feat/redis-read-write-lock branch April 19, 2022 11:52

joachimvh mentioned this pull request Apr 19, 2022

Make redis-based locking mechanism fully multithreaded #325

Closed

joachimvh mentioned this pull request Apr 29, 2022

Implement a multithreaded locking mechanism #309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add a Redis based Read-Write Locker #1263

feat: Add a Redis based Read-Write Locker #1263

Falx commented Apr 13, 2022 •

edited

Loading

Falx commented Apr 13, 2022 •

edited

Loading

joachimvh left a comment

Falx commented Apr 14, 2022

joachimvh commented Apr 14, 2022

Falx commented Apr 15, 2022

joachimvh commented Apr 15, 2022

Falx commented Apr 15, 2022

Falx commented Apr 15, 2022

joachimvh left a comment

joachimvh left a comment

feat: Add a Redis based Read-Write Locker #1263

feat: Add a Redis based Read-Write Locker #1263

Conversation

Falx commented Apr 13, 2022 • edited Loading

📁 Related issues

✍️ Description

✅ PR check list

Falx commented Apr 13, 2022 • edited Loading

joachimvh left a comment

Choose a reason for hiding this comment

Falx commented Apr 14, 2022

joachimvh commented Apr 14, 2022

Falx commented Apr 15, 2022

joachimvh commented Apr 15, 2022

Falx commented Apr 15, 2022

Falx commented Apr 15, 2022

joachimvh left a comment

Choose a reason for hiding this comment

joachimvh left a comment

Choose a reason for hiding this comment

Falx commented Apr 13, 2022 •

edited

Loading

Falx commented Apr 13, 2022 •

edited

Loading