From 5519ec66cd967af4f8f2115d583a7dbf30e0591f Mon Sep 17 00:00:00 2001 From: Amy Li Date: Tue, 1 Dec 2020 14:59:36 -0800 Subject: [PATCH 1/3] Change redis key format to support clusters and add integration tests Add documentation for how to run tests with a redis cluster Add test cluster Ensure backwards compatibility with existing locks bump gem version Create backend to handle multiple masterlocks Fix to pass specs --- .gitignore | 4 + README.md | 20 ++++- cluster-test/7000/redis.conf | 5 ++ cluster-test/7001/redis.conf | 5 ++ cluster-test/7002/redis.conf | 5 ++ cluster-test/7003/redis.conf | 5 ++ cluster-test/7004/redis.conf | 5 ++ cluster-test/7005/redis.conf | 5 ++ lib/master_lock.rb | 92 +++++--------------- lib/master_lock/backend.rb | 125 ++++++++++++++++++++++++++++ lib/master_lock/redis_lock.rb | 8 +- lib/master_lock/version.rb | 2 +- spec/master_lock/redis_lock_spec.rb | 74 +++++++++++++++- spec/spec_helper.rb | 7 +- spec/support/redis_test.rb | 10 +++ 15 files changed, 294 insertions(+), 78 deletions(-) create mode 100644 cluster-test/7000/redis.conf create mode 100644 cluster-test/7001/redis.conf create mode 100644 cluster-test/7002/redis.conf create mode 100644 cluster-test/7003/redis.conf create mode 100644 cluster-test/7004/redis.conf create mode 100644 cluster-test/7005/redis.conf create mode 100644 lib/master_lock/backend.rb diff --git a/.gitignore b/.gitignore index 0cb6eeb..c721bec 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,7 @@ /pkg/ /spec/reports/ /tmp/ +.idea +/dump.rdb +cluster-test/700*/appendonly.aof +cluster-test/700*/nodes.conf diff --git a/README.md b/README.md index c5a5cf8..4265288 100644 --- a/README.md +++ b/README.md @@ -40,9 +40,27 @@ See [documentation](http://www.rubydoc.info/gems/master_lock) for advanced usage ## Development -After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment. +After checking out the repo, run `bundle install` to install the gem dependencies. To install this gem onto your local machine, run `bundle exec rake install`. To release a new version, update the version number in `version.rb`, and then run `bundle exec rake release`, which will create a git tag for the version, push git commits and tags, and push the `.gem` file to [rubygems.org](https://rubygems.org). +### Testing +If you do not have Redis set up, run `brew install redis`. This gives you access to `redis-server`. + +To set up the redis instance, run `redis-server` in the project level directory. The default config should be located at `/usr/local/etc/redis.conf`. + +To set up the redis cluster, copy your redis-server executable to `cluster-test/redis-server`. Open up 6 terminal tabs, and in every tab, start every instance: +``` +cd cluster-test/7000 +../redis-server ./redis.conf +``` +Assuming you have at least Redis 5, create your cluster by running the following: +``` +redis-cli --cluster create 127.0.0.1:7000 127.0.0.1:7001 \ +127.0.0.1:7002 127.0.0.1:7003 127.0.0.1:7004 127.0.0.1:7005 \ +--cluster-replicas 1 +``` + +Then, run `rake spec` to run the tests. ## Contributing diff --git a/cluster-test/7000/redis.conf b/cluster-test/7000/redis.conf new file mode 100644 index 0000000..31937a5 --- /dev/null +++ b/cluster-test/7000/redis.conf @@ -0,0 +1,5 @@ +port 7000 +cluster-enabled yes +cluster-config-file nodes.conf +cluster-node-timeout 5000 +appendonly yes diff --git a/cluster-test/7001/redis.conf b/cluster-test/7001/redis.conf new file mode 100644 index 0000000..689dc11 --- /dev/null +++ b/cluster-test/7001/redis.conf @@ -0,0 +1,5 @@ +port 7001 +cluster-enabled yes +cluster-config-file nodes.conf +cluster-node-timeout 5000 +appendonly yes diff --git a/cluster-test/7002/redis.conf b/cluster-test/7002/redis.conf new file mode 100644 index 0000000..ac61065 --- /dev/null +++ b/cluster-test/7002/redis.conf @@ -0,0 +1,5 @@ +port 7002 +cluster-enabled yes +cluster-config-file nodes.conf +cluster-node-timeout 5000 +appendonly yes diff --git a/cluster-test/7003/redis.conf b/cluster-test/7003/redis.conf new file mode 100644 index 0000000..5493390 --- /dev/null +++ b/cluster-test/7003/redis.conf @@ -0,0 +1,5 @@ +port 7003 +cluster-enabled yes +cluster-config-file nodes.conf +cluster-node-timeout 5000 +appendonly yes diff --git a/cluster-test/7004/redis.conf b/cluster-test/7004/redis.conf new file mode 100644 index 0000000..84959a3 --- /dev/null +++ b/cluster-test/7004/redis.conf @@ -0,0 +1,5 @@ +port 7004 +cluster-enabled yes +cluster-config-file nodes.conf +cluster-node-timeout 5000 +appendonly yes diff --git a/cluster-test/7005/redis.conf b/cluster-test/7005/redis.conf new file mode 100644 index 0000000..b8dd323 --- /dev/null +++ b/cluster-test/7005/redis.conf @@ -0,0 +1,5 @@ +port 7005 +cluster-enabled yes +cluster-config-file nodes.conf +cluster-node-timeout 5000 +appendonly yes diff --git a/lib/master_lock.rb b/lib/master_lock.rb index d11e499..8c90b2d 100644 --- a/lib/master_lock.rb +++ b/lib/master_lock.rb @@ -1,4 +1,5 @@ require 'master_lock/version' +require 'master_lock/backend' require 'logger' require 'socket' @@ -13,6 +14,7 @@ # alive, a separate thread will extend the lifetime of the locks so that they # do not expire even when the code in the critical section takes a long time to # execute. +# todo: change to class? module MasterLock class UnconfiguredError < StandardError; end class NotStartedError < StandardError; end @@ -32,10 +34,18 @@ class LockNotAcquiredError < StandardError; end :key_prefix, :redis, :sleep_time, - :ttl + :ttl, + :cluster ) class << self + def backend + @backend ||= Backend.new + end + + def backend=(backend) + @backend = backend + end # Obtain a mutex around a critical section of code. Only one thread on any # machine can execute the given block at a time. Returns the result of the # block. @@ -55,105 +65,41 @@ class << self # @raise [NotStartedError] if called before {#start} # @raise [LockNotAcquiredError] if the lock cannot be acquired before the # timeout - def synchronize(key, options = {}) - check_configured - raise NotStartedError unless @registry - - ttl = options[:ttl] || config.ttl - acquire_timeout = options[:acquire_timeout] || config.acquire_timeout - extend_interval = options[:extend_interval] || config.extend_interval - - raise ArgumentError, "extend_interval cannot be negative" if extend_interval < 0 - raise ArgumentError, "ttl must be greater extend_interval" if ttl <= extend_interval - - if (options.include?(:if) && !options[:if]) || - (options.include?(:unless) && options[:unless]) - return yield - end - - lock = RedisLock.new( - redis: config.redis, - key: key, - ttl: ttl, - owner: generate_owner - ) - if !lock.acquire(timeout: acquire_timeout) - raise LockNotAcquiredError, key - end - - registration = - @registry.register(lock, extend_interval) - logger.debug("Acquired lock #{key}") - begin - yield - ensure - @registry.unregister(registration) - if lock.release - logger.debug("Released lock #{key}") - else - logger.warn("Failed to release lock #{key}") - end - end + def synchronize(key, options = {}, &blk) + backend.synchronize(key, options, &blk) end # Starts the background thread to manage and extend currently held locks. # The thread remains alive for the lifetime of the process. This must be # called before any locks may be acquired. def start - @registry = Registry.new - Thread.new do - loop do - @registry.extend_locks - sleep(config.sleep_time) - end - end + backend.start end # Returns true if the registry has been started, otherwise false # @return [Boolean] def started? - !@registry.nil? + backend.started? end # Get the configured logger. # # @return [Logger] def logger - config.logger + backend.logger end # @return [Config] MasterLock configuration settings def config - if !defined?(@config) - @config = Config.new - @config.acquire_timeout = DEFAULT_ACQUIRE_TIMEOUT - @config.extend_interval = DEFAULT_EXTEND_INTERVAL - @config.hostname = Socket.gethostname - @config.logger = Logger.new(STDOUT) - @config.logger.progname = name - @config.key_prefix = DEFAULT_KEY_PREFIX - @config.sleep_time = DEFAULT_SLEEP_TIME - @config.ttl = DEFAULT_TTL - end - @config + backend.config end # Configure MasterLock using block syntax. Simply yields {#config} to the # block. # # @yield [Config] the configuration - def configure - yield config - end - - private - - def check_configured - raise UnconfiguredError, "redis must be configured" unless config.redis - end - - def generate_owner - "#{config.hostname}:#{Process.pid}:#{Thread.current.object_id}" + def configure(&blk) + backend.configure(&blk) end end end diff --git a/lib/master_lock/backend.rb b/lib/master_lock/backend.rb new file mode 100644 index 0000000..a8223f0 --- /dev/null +++ b/lib/master_lock/backend.rb @@ -0,0 +1,125 @@ +module MasterLock + class Backend + + # Obtain a mutex around a critical section of code. Only one thread on any + # machine can execute the given block at a time. Returns the result of the + # block. + # + # @param key [String] the unique identifier for the locked resource + # @option options [Fixnum] :ttl (60) the length of time in seconds before + # the lock expires + # @option options [Fixnum] :acquire_timeout (5) the length of time to wait + # to acquire the lock before timing out + # @option options [Fixnum] :extend_interval (15) the amount of time in + # seconds that may pass before extending the lock + # @option options [Boolean] :if if this option is falsey, the block will be + # executed without obtaining the lock + # @option options [Boolean] :unless if this option is truthy, the block will + # be executed without obtaining the lock + # @raise [UnconfiguredError] if a required configuration variable is unset + # @raise [NotStartedError] if called before {#start} + # @raise [LockNotAcquiredError] if the lock cannot be acquired before the + # timeout + def synchronize(key, options = {}) + check_configured + raise NotStartedError unless @registry + + ttl = options[:ttl] || config.ttl + acquire_timeout = options[:acquire_timeout] || config.acquire_timeout + extend_interval = options[:extend_interval] || config.extend_interval + + raise ArgumentError, "extend_interval cannot be negative" if extend_interval < 0 + raise ArgumentError, "ttl must be greater extend_interval" if ttl <= extend_interval + + if (options.include?(:if) && !options[:if]) || + (options.include?(:unless) && options[:unless]) + return yield + end + + lock = RedisLock.new( + redis: config.redis, + key: key, + ttl: ttl, + owner: generate_owner + ) + if !lock.acquire(timeout: acquire_timeout) + raise LockNotAcquiredError, key + end + + registration = + @registry.register(lock, extend_interval) + logger.debug("Acquired lock #{key}") + begin + yield + ensure + @registry.unregister(registration) + if lock.release + logger.debug("Released lock #{key}") + else + logger.warn("Failed to release lock #{key}") + end + end + end + + # Starts the background thread to manage and extend currently held locks. + # The thread remains alive for the lifetime of the process. This must be + # called before any locks may be acquired. + def start + @registry = Registry.new + Thread.new do + loop do + @registry.extend_locks + sleep(config.sleep_time) + end + end + end + + # Returns true if the registry has been started, otherwise false + # @return [Boolean] + def started? + !@registry.nil? + end + + # Get the configured logger. + # + # @return [Logger] + def logger + config.logger + end + + # @return [Config] MasterLock configuration settings + def config + if !defined?(@config) + @config = Config.new + @config.acquire_timeout = DEFAULT_ACQUIRE_TIMEOUT + @config.extend_interval = DEFAULT_EXTEND_INTERVAL + @config.hostname = Socket.gethostname + @config.logger = Logger.new(STDOUT) + @config.logger.progname = 'MasterLock' + @config.key_prefix = DEFAULT_KEY_PREFIX + @config.sleep_time = DEFAULT_SLEEP_TIME + @config.ttl = DEFAULT_TTL + @config.cluster = false + end + @config + end + + # Configure MasterLock using block syntax. Simply yields {#config} to the + # block. + # + # @yield [Config] the configuration + def configure + yield config + end + + private + + def check_configured + raise UnconfiguredError, "redis must be configured" unless config.redis + end + + def generate_owner + "#{config.hostname}:#{Process.pid}:#{Thread.current.object_id}" + end + end +end \ No newline at end of file diff --git a/lib/master_lock/redis_lock.rb b/lib/master_lock/redis_lock.rb index d9c986e..6fdfb78 100644 --- a/lib/master_lock/redis_lock.rb +++ b/lib/master_lock/redis_lock.rb @@ -95,7 +95,13 @@ def eval_script(script, script_hash, keys:, argv:) end def redis_key - "#{MasterLock.config.key_prefix}:#{key}" + # Key hash tags are a way to ensure multiple keys are allocated in the same hash slot. + # This allows our redis operations to work with clusters + if MasterLock.config.cluster + "{#{MasterLock.config.key_prefix}}:#{key}" + else + "#{MasterLock.config.key_prefix}:#{key}" + end end end end diff --git a/lib/master_lock/version.rb b/lib/master_lock/version.rb index 9ca9d69..be2ea9e 100644 --- a/lib/master_lock/version.rb +++ b/lib/master_lock/version.rb @@ -1,3 +1,3 @@ module MasterLock - VERSION = "0.11.1" + VERSION = "0.11.2" end diff --git a/spec/master_lock/redis_lock_spec.rb b/spec/master_lock/redis_lock_spec.rb index 836aa2f..f5459bd 100644 --- a/spec/master_lock/redis_lock_spec.rb +++ b/spec/master_lock/redis_lock_spec.rb @@ -1,31 +1,66 @@ require 'spec_helper' -RSpec.describe MasterLock::RedisLock, redis: true do +RSpec.describe MasterLock::RedisLock, redis: true, cluster: true do let(:lock1) do described_class.new(redis: redis, key: "key", owner: "owner1", ttl: 0.1) end let(:lock2) do described_class.new(redis: redis, key: "key", owner: "owner2", ttl: 0.1) end + let(:lock3) do + described_class.new(redis: cluster, key: "key", owner: "owner3", ttl: 0.1) + end + let(:lock4) do + described_class.new(redis: cluster, key: "key", owner: "owner4", ttl: 0.1) + end + + before do + MasterLock.config.cluster = false + end + + describe "#cluster basic tests" do + it "fails to get the keys" do + expect{ cluster.mget('key1', 'key2') }.to raise_error(Redis::CommandError) + end + + it "successfully gets the keys" do + expect { cluster.mget('{key}1','{key}2')}.to_not raise_error + end + end describe "#acquire" do it "returns true when lock can be acquired" do expect(lock1.acquire(timeout: 0)).to eq(true) + # Cluster test + MasterLock.config.cluster = true + expect(lock3.acquire(timeout: 0)).to eq(true) end it "returns false when lock can not be acquired" do expect(lock1.acquire(timeout: 0)).to eq(true) expect(lock2.acquire(timeout: 0)).to eq(false) + # Cluster test + MasterLock.config.cluster = true + expect(lock3.acquire(timeout: 0)).to eq(true) + expect(lock4.acquire(timeout: 0)).to eq(false) end it "returns false if the same owner already has the lock" do expect(lock1.acquire(timeout: 0)).to eq(true) expect(lock1.acquire(timeout: 0)).to eq(false) + # Cluster test + MasterLock.config.cluster = true + expect(lock3.acquire(timeout: 0)).to eq(true) + expect(lock3.acquire(timeout: 0)).to eq(false) end it "attempts to acquire the lock repeatedly until timeout" do expect(lock1.acquire(timeout: 0)).to eq(true) expect(lock2.acquire(timeout: 1)).to eq(true) + # Cluster test + MasterLock.config.cluster = true + expect(lock3.acquire(timeout: 0)).to eq(true) + expect(lock4.acquire(timeout: 1)).to eq(true) end end @@ -33,15 +68,26 @@ it "returns true when lock is held by owner" do lock1.acquire(timeout: 0) expect(lock1.extend).to eq(true) + # Cluster test + MasterLock.config.cluster = true + lock3.acquire(timeout: 0) + expect(lock3.extend).to eq(true) end it "returns false when lock is held by another owner" do lock2.acquire(timeout: 0) expect(lock1.extend).to eq(false) + # Cluster test + MasterLock.config.cluster = true + lock4.acquire(timeout: 0) + expect(lock3.extend).to eq(false) end it "returns false when lock is not held" do expect(lock1.extend).to eq(false) + # Cluster test + MasterLock.config.cluster = true + expect(lock3.extend).to eq(false) end it "resets the expiration time" do @@ -52,6 +98,15 @@ end sleep 0.2 expect(lock1.extend).to eq(false) + # Cluster test + MasterLock.config.cluster = true + lock3.acquire(timeout: 0) + 3.times do + sleep 0.05 + expect(lock3.extend).to eq(true) + end + sleep 0.2 + expect(lock3.extend).to eq(false) end end @@ -59,15 +114,26 @@ it "returns true when lock is held by owner" do lock1.acquire(timeout: 0) expect(lock1.release).to eq(true) + # Cluster test + MasterLock.config.cluster = true + lock3.acquire(timeout: 0) + expect(lock3.release).to eq(true) end it "returns false when lock is held by another owner" do lock2.acquire(timeout: 0) expect(lock1.release).to eq(false) + # Cluster test + MasterLock.config.cluster = true + lock4.acquire(timeout: 0) + expect(lock3.release).to eq(false) end it "returns false when lock is not held" do expect(lock1.release).to eq(false) + # Cluster test + MasterLock.config.cluster = true + expect(lock3.release).to eq(false) end it "changes the lock to be unowned" do @@ -75,6 +141,12 @@ expect(lock1.release).to eq(true) expect(lock1.release).to eq(false) expect(lock2.acquire(timeout: 0)).to eq(true) + # Cluster test + MasterLock.config.cluster = true + lock3.acquire(timeout: 0) + expect(lock3.release).to eq(true) + expect(lock3.release).to eq(false) + expect(lock4.acquire(timeout: 0)).to eq(true) end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 176e0db..d393532 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -62,13 +62,18 @@ # as the one that triggered the failure. Kernel.srand config.seed - config.include RedisTest, redis: true + config.include RedisTest, redis: true, cluster: true config.before(:each, redis: true) do clean_redis end + + config.before(:each, cluster: true) do + clean_cluster + end end MasterLock.configure do |config| config.logger.level = :info + config.cluster = false end diff --git a/spec/support/redis_test.rb b/spec/support/redis_test.rb index 1ff4abf..77a61ff 100644 --- a/spec/support/redis_test.rb +++ b/spec/support/redis_test.rb @@ -7,8 +7,18 @@ def redis @redis ||= Redis.new(url: REDIS_URL) end + def cluster + nodes = (7000..7005).map { |port| "redis://127.0.0.1:#{port}" } + @cluster ||= Redis.new(cluster: nodes) + end + def clean_redis redis.flushdb redis.script(:flush) end + + def clean_cluster + cluster.flushdb + cluster.script(:flush) + end end From 114bbbcafbc4294e2920f8ddb9e585addc6c2bb9 Mon Sep 17 00:00:00 2001 From: Amy Li Date: Tue, 23 Feb 2021 09:24:38 -1000 Subject: [PATCH 2/3] Remove unnecessary comment --- lib/master_lock.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/master_lock.rb b/lib/master_lock.rb index 8c90b2d..0752a17 100644 --- a/lib/master_lock.rb +++ b/lib/master_lock.rb @@ -14,7 +14,6 @@ # alive, a separate thread will extend the lifetime of the locks so that they # do not expire even when the code in the critical section takes a long time to # execute. -# todo: change to class? module MasterLock class UnconfiguredError < StandardError; end class NotStartedError < StandardError; end From 1ab3ea7f956d214c9e8e6816bb294eb9ad4958f3 Mon Sep 17 00:00:00 2001 From: Amy Li Date: Tue, 23 Feb 2021 13:54:20 -1000 Subject: [PATCH 3/3] Bump minor version --- lib/master_lock/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/master_lock/version.rb b/lib/master_lock/version.rb index be2ea9e..c5c4c4f 100644 --- a/lib/master_lock/version.rb +++ b/lib/master_lock/version.rb @@ -1,3 +1,3 @@ module MasterLock - VERSION = "0.11.2" + VERSION = "0.12.0" end