toString is globally polluted #2

Closed
evantahler opened this Issue Feb 22, 2013 · 15 comments

Projects

None yet

2 participants

@evantahler

It looks like the current version of fakeredis polutes the global definition of toString:

console.log(toString.toString()) 
// GOOD
// function toString() { [native code] }

var fakeredis = require('fakeredis');
var client = fakeredis.createClient(); 
console.log(toString.toString()) 
// BAD
// function () { return "<TYPE<" + type + ">>"; }
@hdachev
Owner
hdachev commented Feb 22, 2013

Very bad indeed, will look into it later today!
On Feb 22, 2013 8:28 AM, "Evan Tahler" notifications@github.com wrote:

It looks like the current version of fakeredis polutes the global
definition of toString:

console.log(toString.toString()) // GOOD// function toString() { [native code] }
var fakeredis = require('fakeredis');var client = fakeredis.createClient(); console.log(toString.toString())
// BAD// function () { return "<TYPE<" + type + ">>"; }


Reply to this email directly or view it on GitHubhttps://github.com/hdachev/fakeredis/issues/2.

@hdachev
Owner
hdachev commented Feb 22, 2013

Fixed, can you pull and confirm it's good before we publish to npm?

@evantahler

my test above is OK! Push it up to NPM!

FYI, I learned about this bug while running a mocha test suite which detects global leaks http://visionmedia.github.com/mocha/. I'm hoping to be able to use fakeredis in http://actionherojs.com/ in a "single node" option to allow folks to run the server in development mode w/o real redis installed. I've got tons of if(redis.enable === true){ }else{ } code which I want to remove.

@evantahler evantahler closed this Feb 22, 2013
@hdachev
Owner
hdachev commented Feb 22, 2013

Sounds great! Are you targeting Redis 2.6+? We're still stuck with 2.4 in production, so I'm not sure I've covered all of the new features. Need to look into that. Meanwhile looks like I have missed some incompatibility introduced between the redis@0.7.2 and redis@0.8.x clients related to hash commands, which needs to be fixed too.

I'll let you know when I have something tomorrow or the day after.

@evantahler

I should only need ~2.4 (I think). The only real issue I noticed was that in 'real' redis I can safely hdel from a hash which doesn't exist without an exception (but there is an error returned). In fakeredis I get an exception

@hdachev
Owner
hdachev commented Feb 22, 2013

I can't reproduce, can you provide an example?

@evantahler

Oops, it was lrem

Here's a simplified example of a server's boot behavior (it checks to make sure that there isn't an entry from a crashed version of itself):

var fakeredis = require('fakeredis');
var client = fakeredis.createClient(); 
var myName = "123abc";

client.lrem("someList", 1, myName, function(err, count){ // this breaks
  client.hdel("someHash", myName, function(){ // this is OK
    console.log("DONE")
  });
});
@hdachev
Owner
hdachev commented Feb 23, 2013

I've pushed to npm. Let me know if you spot something else or if you're missing some Redis 2.6.x command, apart from Lua scripting everything should be trivial to implement.

Meanwhile, I was thinking about your use case for fakeredis for development actionHero setups, currently the simulated connection latency is anywhere around 20 and 50 ms, which is beyond pessimistic, let me know if you find this to be a problem, we can surface these things as options.

@evantahler

Thanks!

Frankly, I want people to use real redis, at least in production. It's pretty integral to the system (peer-2-peer node communication, shared cache, etc). However, I want to allow a redis-less development environment and "micro" deploys which only use some of the functionality of the framework. Increased latency seems OK for now.

@evantahler

And I've got fake redis working as a drop-in replacement for redis in actionHero!

You were correct that is is rather slower. I have some benchmarks in the test suite to ensure speed (IE: 1000 web requests which test the redis-backed cache should take under 1 second). Some actions (like reading and writing to a hash) are ~ 10x slower, but every action I need works!

Thanks again!

@hdachev
Owner
hdachev commented Feb 24, 2013

I'll read through the code and see what I can extract as options. I don't want to just reduce the latency for everyone, because it helps reproducing race-condition scenarios between multiple clients when testing data-structure stuff etc, but since you obviously don't need that, you should have the option to opt out of the excessive waiting.

In any case, I advise against fakeredis in production, it's just too weird.

@evantahler

Totally agree :D

@hdachev
Owner
hdachev commented Feb 25, 2013

So this is what I did.

First I added a .fast option for .createClient() that lets you opt out of the excessive latency, but then I looked at your code for actionHero and I saw that you're swapping the redis package for fakeredis before instantiating any clients, and that would require additional if(api.redis.fake) checks, which wouldn't be amazingly elegant, so I also added a global .fast option on the package itself. The full simulated roundtrip latency should drop from 30-60ms to about 2ms when setting either of those.

Does it do the trick?

@evantahler

That's awesome! Thanks so much!
Let me know when it's up on NPM, and I'll plug it in

@hdachev
Owner
hdachev commented Feb 26, 2013

It has been done. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment