Skip to content

Commit

Permalink
Fixed Group Cluster key reuse after GC
Browse files Browse the repository at this point in the history
  • Loading branch information
Simon Grondin committed Mar 24, 2018
1 parent 13e1d6c commit a9151ff
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 42 deletions.
30 changes: 20 additions & 10 deletions bottleneck.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1032,6 +1032,7 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
var r, redis;
this.loadAll = this.loadAll.bind(this);
this.instance = instance;
this.initSettings = initSettings;
r = require;
redis = r(function () {
return ["r", "e", "d", "i", "s"].join(""); // Obfuscated or else Webpack/Angular will try to inline the optional redis module
Expand Down Expand Up @@ -1092,14 +1093,7 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
return this.instance._drainAll(~~info);
}
});
initSettings.id = this.originalId;
initSettings.nextRequest = Date.now();
initSettings.running = 0;
initSettings.unblockTime = 0;
initSettings.version = this.instance.version;
initSettings.groupTimeout = this._groupTimeout;
args = this.prepareObject(initSettings);
args.unshift(options.clearDatastore ? 1 : 0);
args = this.prepareInitSettings(options.clearDatastore);
this.isReady = true;
return this.runScript("init", args);
}).then(results => {
Expand Down Expand Up @@ -1163,10 +1157,24 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
return arr;
}

prepareInitSettings(clear) {
var args;
args = this.prepareObject(Object.assign({}, this.initSettings, {
id: this.originalId,
nextRequest: Date.now(),
running: 0,
unblockTime: 0,
version: this.instance.version,
groupTimeout: this._groupTimeout
}));
args.unshift(clear ? 1 : 0);
return args;
}

runScript(name, args) {
var script;
if (!this.isReady) {
return this.Promise.reject(new BottleneckError("This limiter is not done connecting to Redis yet. Wait for the 'ready' event to be triggered before submitting requests."));
return this.Promise.reject(new BottleneckError("This limiter is not done connecting to Redis yet. Wait for the '.ready()' promise to resolve before submitting requests."));
} else {
script = this.scripts[name];
return new this.Promise((resolve, reject) => {
Expand All @@ -1181,7 +1189,9 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
return this.client.evalsha.bind(this.client).apply({}, arr);
}).catch(e => {
if (e.message === "SETTINGS_KEY_NOT_FOUND") {
return this.Promise.reject(new BottleneckError(`Bottleneck limiter (id: '${this.originalId}') could not find the Redis key it needs to complete this action (key '${script.keys[0]}'), was it deleted?${this._groupTimeout != null ? ' Note: This limiter is in a Group, it could have been garbage collected.' : ''}`));
return this.runScript("init", this.prepareInitSettings(false)).then(() => {
return this.runScript(name, args);
});
} else {
return this.Promise.reject(e);
}
Expand Down
2 changes: 1 addition & 1 deletion bottleneck.min.js
View file

Large diffs are not rendered by default.

30 changes: 20 additions & 10 deletions lib/RedisStorage.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
var r, redis;
this.loadAll = this.loadAll.bind(this);
this.instance = instance;
this.initSettings = initSettings;
r = require;
redis = r(function () {
return ["r", "e", "d", "i", "s"].join(""); // Obfuscated or else Webpack/Angular will try to inline the optional redis module
Expand Down Expand Up @@ -144,14 +145,7 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
return this.instance._drainAll(~~info);
}
});
initSettings.id = this.originalId;
initSettings.nextRequest = Date.now();
initSettings.running = 0;
initSettings.unblockTime = 0;
initSettings.version = this.instance.version;
initSettings.groupTimeout = this._groupTimeout;
args = this.prepareObject(initSettings);
args.unshift(options.clearDatastore ? 1 : 0);
args = this.prepareInitSettings(options.clearDatastore);
this.isReady = true;
return this.runScript("init", args);
}).then(results => {
Expand Down Expand Up @@ -215,10 +209,24 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
return arr;
}

prepareInitSettings(clear) {
var args;
args = this.prepareObject(Object.assign({}, this.initSettings, {
id: this.originalId,
nextRequest: Date.now(),
running: 0,
unblockTime: 0,
version: this.instance.version,
groupTimeout: this._groupTimeout
}));
args.unshift(clear ? 1 : 0);
return args;
}

runScript(name, args) {
var script;
if (!this.isReady) {
return this.Promise.reject(new BottleneckError("This limiter is not done connecting to Redis yet. Wait for the 'ready' event to be triggered before submitting requests."));
return this.Promise.reject(new BottleneckError("This limiter is not done connecting to Redis yet. Wait for the '.ready()' promise to resolve before submitting requests."));
} else {
script = this.scripts[name];
return new this.Promise((resolve, reject) => {
Expand All @@ -233,7 +241,9 @@ function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, a
return this.client.evalsha.bind(this.client).apply({}, arr);
}).catch(e => {
if (e.message === "SETTINGS_KEY_NOT_FOUND") {
return this.Promise.reject(new BottleneckError(`Bottleneck limiter (id: '${this.originalId}') could not find the Redis key it needs to complete this action (key '${script.keys[0]}'), was it deleted?${this._groupTimeout != null ? ' Note: This limiter is in a Group, it could have been garbage collected.' : ''}`));
return this.runScript("init", this.prepareInitSettings(false)).then(() => {
return this.runScript(name, args);
});
} else {
return this.Promise.reject(e);
}
Expand Down
28 changes: 16 additions & 12 deletions src/RedisStorage.coffee
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ scriptTemplates = (id) ->
code: lua["increment_reservoir.lua"]

class RedisStorage
constructor: (@instance, initSettings, options) ->
constructor: (@instance, @initSettings, options) ->
r = require
redis = r do -> ["r", "e", "d", "i", "s"].join("") # Obfuscated or else Webpack/Angular will try to inline the optional redis module
@originalId = @instance.id
Expand Down Expand Up @@ -86,15 +86,7 @@ class RedisStorage
[type, info] = message.split ":"
if type == "freed" then @instance._drainAll ~~info

initSettings.id = @originalId
initSettings.nextRequest = Date.now()
initSettings.running = 0
initSettings.unblockTime = 0
initSettings.version = @instance.version
initSettings.groupTimeout = @_groupTimeout

args = @prepareObject(initSettings)
args.unshift (if options.clearDatastore then 1 else 0)
args = @prepareInitSettings options.clearDatastore
@isReady = true
@runScript "init", args
.then (results) =>
Expand Down Expand Up @@ -124,8 +116,20 @@ class RedisStorage
for k, v of obj then arr.push k, (if v? then v.toString() else "")
arr

prepareInitSettings: (clear) ->
args = @prepareObject Object.assign({}, @initSettings, {
id: @originalId,
nextRequest: Date.now(),
running: 0,
unblockTime: 0,
version: @instance.version,
groupTimeout: @_groupTimeout
})
args.unshift (if clear then 1 else 0)
args

runScript: (name, args) ->
if !@isReady then @Promise.reject new BottleneckError "This limiter is not done connecting to Redis yet. Wait for the 'ready' event to be triggered before submitting requests."
if !@isReady then @Promise.reject new BottleneckError "This limiter is not done connecting to Redis yet. Wait for the '.ready()' promise to resolve before submitting requests."
else
script = @scripts[name]
new @Promise (resolve, reject) =>
Expand All @@ -136,7 +140,7 @@ class RedisStorage
@client.evalsha.bind(@client).apply {}, arr
.catch (e) =>
if e.message == "SETTINGS_KEY_NOT_FOUND"
then @Promise.reject new BottleneckError "Bottleneck limiter (id: '#{@originalId}') could not find the Redis key it needs to complete this action (key '#{script.keys[0]}'), was it deleted?#{if @_groupTimeout? then ' Note: This limiter is in a Group, it could have been garbage collected.' else ''}"
then @runScript("init", @prepareInitSettings(false)).then => @runScript(name, args)
else @Promise.reject e

convertBool: (b) -> !!b
Expand Down
15 changes: 6 additions & 9 deletions test/redis.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ if (process.env.DATASTORE === 'redis') {
return Promise.reject('Should not have been ready')
})
.catch(function (err) {
c.mustEqual(err.message, 'This limiter is not done connecting to Redis yet. Wait for the \'ready\' event to be triggered before submitting requests.')
c.mustEqual(err.message, 'This limiter is not done connecting to Redis yet. Wait for the \'.ready()\' promise to resolve before submitting requests.')
return Promise.resolve()
})
})
Expand Down Expand Up @@ -222,11 +222,11 @@ if (process.env.DATASTORE === 'redis') {
})
})

it('Should fail when Redis data is missing', function (done) {
it('Should not fail when Redis data is missing', function () {
c = makeTest()
var limiter = new Bottleneck({ datastore: 'redis', clearDatastore: true })

limiter.ready()
return limiter.ready()
.then(function () {
return limiter.running()
})
Expand All @@ -237,20 +237,18 @@ if (process.env.DATASTORE === 'redis') {
return new Promise(function (resolve, reject) {
limiter._store.client.del(settings_key, function (err, data) {
if (err != null) return reject(err)
c.mustEqual(data, 1) // Should be 1, since 1 key should have been deleted
return resolve(data)
})
})

})
.then(function (deleted) {
c.mustEqual(deleted, 1)
c.mustEqual(deleted, 1) // Should be 1, since 1 key should have been deleted
return limiter.running()
})
.catch(function (err) {
c.mustEqual(err.message, 'Bottleneck limiter (id: \'<no-id>\') could not find the Redis key it needs to complete this action (key \'b_<no-id>_settings\'), was it deleted?')
.then(function (running) {
c.mustEqual(running, 0)
limiter.disconnect(false)
done()
})
})

Expand Down Expand Up @@ -339,7 +337,6 @@ if (process.env.DATASTORE === 'redis') {
})
})


})

})
Expand Down

0 comments on commit a9151ff

Please sign in to comment.