Skip to content

Commit 8b7d4a8

Browse files
author
Ruben Bridgewater
committed
Remove bad .eval implementation
The implementation is not really good as mentioned in #722 and we pipline our commands. That way we can't just replace the eval function as it was. This could result in violating the order of execution! If we want to include a function like this we should not break the order of execution and also not recalculate the sha1 hash each time.
1 parent 13635c9 commit 8b7d4a8

File tree

2 files changed

+20
-41
lines changed

2 files changed

+20
-41
lines changed

index.js

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ var net = require("net"),
66
Queue = require("./lib/queue"),
77
to_array = require("./lib/to_array"),
88
events = require("events"),
9-
crypto = require("crypto"),
109
parsers = [],
1110
// This static list of commands is updated from time to time.
1211
// ./lib/commands.js can be updated with generate_commands.js
@@ -1083,37 +1082,6 @@ RedisClient.prototype.multi = RedisClient.prototype.MULTI = function (args) {
10831082
return new Multi(this, args);
10841083
};
10851084

1086-
// stash original eval method
1087-
var eval_orig = RedisClient.prototype.eval;
1088-
// hook eval with an attempt to evalsha for cached scripts
1089-
RedisClient.prototype.eval = RedisClient.prototype.EVAL = function () {
1090-
var self = this,
1091-
args = to_array(arguments),
1092-
callback;
1093-
1094-
if (typeof args[args.length - 1] === "function") {
1095-
callback = args.pop();
1096-
}
1097-
1098-
if (Array.isArray(args[0])) {
1099-
args = args[0];
1100-
}
1101-
1102-
// replace script source with sha value
1103-
var source = args[0];
1104-
args[0] = crypto.createHash("sha1").update(source).digest("hex");
1105-
1106-
self.evalsha(args, function (err, reply) {
1107-
if (err && /NOSCRIPT/.test(err.message)) {
1108-
args[0] = source;
1109-
eval_orig.call(self, args, callback);
1110-
1111-
} else if (callback) {
1112-
callback(err, reply);
1113-
}
1114-
});
1115-
};
1116-
11171085
var createClient_unix = function(path, options){
11181086
var cnxOptions = {
11191087
path: path

test/commands/eval.spec.js

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ describe("The 'eval' method", function () {
1212

1313
describe("using " + parser + " and " + ip, function () {
1414
var client;
15+
var source = "return redis.call('set', 'sha', 'test')";
1516

1617
beforeEach(function (done) {
1718
client = redis.createClient.apply(redis.createClient, args);
@@ -94,31 +95,41 @@ describe("The 'eval' method", function () {
9495
});
9596
});
9697

98+
it('allows a script to be executed that accesses the redis API without callback', function (done) {
99+
helper.serverVersionAtLeast.call(this, client, [2, 5, 0]);
100+
client.eval(source, 0);
101+
client.get('sha', helper.isString('test', done));
102+
});
103+
97104
describe('evalsha', function () {
98-
var source = "return redis.call('get', 'sha test')";
99105
var sha = crypto.createHash('sha1').update(source).digest('hex');
100106

101-
beforeEach(function (done) {
102-
client.set("sha test", "eval get sha test", function (err, res) {
103-
return done(err);
104-
});
105-
});
106-
107107
it('allows a script to be executed that accesses the redis API', function (done) {
108108
helper.serverVersionAtLeast.call(this, client, [2, 5, 0]);
109-
client.eval(source, 0, helper.isString('eval get sha test', done));
109+
client.eval(source, 0, helper.isString('OK'));
110+
client.get('sha', helper.isString('test', done));
110111
});
111112

112113
it('can execute a script if the SHA exists', function (done) {
113114
helper.serverVersionAtLeast.call(this, client, [2, 5, 0]);
114-
client.evalsha(sha, 0, helper.isString('eval get sha test', done));
115+
client.evalsha(sha, 0, helper.isString('OK'));
116+
client.get('sha', helper.isString('test', done));
115117
});
116118

117119
it('returns an error if SHA does not exist', function (done) {
118120
helper.serverVersionAtLeast.call(this, client, [2, 5, 0]);
119121
client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0, helper.isError(done));
120122
});
121123

124+
it('emit an error if SHA does not exist without any callback', function (done) {
125+
helper.serverVersionAtLeast.call(this, client, [2, 5, 0]);
126+
client.evalsha('ffffffffffffffffffffffffffffffffffffffff', 0);
127+
client.on('error', function(err) {
128+
assert(/NOSCRIPT No matching script. Please use EVAL./.test(err.message));
129+
done();
130+
});
131+
});
132+
122133
it('emits an error if SHA does not exist and no callback has been provided', function (done) {
123134
client.on('error', function (err) {
124135
assert.equal(err.message, 'NOSCRIPT No matching script. Please use EVAL.');

0 commit comments

Comments
 (0)