bminer / node-mysql-queues

Wraps 'node-mysql' to provide mulitple query queues, allowing support for multiple statements and transactions.
MIT License
92 stars 11 forks source link

Not being allowed to do async makes it useless #6

Closed ronkorving closed 12 years ago

ronkorving commented 12 years ago

While I love the idea behind node-mysql-queues, I won't be able to use it, simply due to the requirement that inside of a query callback, one is not allowed to do anything asynchronous. This is an unreasonable requirement in the asynchronous world that is NodeJS, and will no doubt cause many users to become clueless as to why things aren't working. The mentioned workarounds are hacks, at best.

My feature (removal) request:

Why not simply remove the auto-commit behavior, and rely on the developer to always commit or rollback? This magic that is auto-commit, (apparently accompanied by warnings, so who would want to use it anyway?) is useless. Yet it creates the "no async for you buddy" constraint, which is a huge anti-feature for everybody.

bminer commented 12 years ago

Agreed. This constraint of "no async in query callbacks" was removed in 0.3.1 and improved in 0.3.2. This just happened a few hours ago. :) Checkout the pause() method.

The auto-commit feature almost "has to be there" because node-mysql-queues has to return control to the main node-mysql queue. I suppose I could auto-rollback instead... but something has to be done. Otherwise, a wild mess occurs where all queries that were in the main queue waiting to be executed are now part of a transaction that was created in a separate scope of the program, making it nearly impossible to debug. Anyway... hopefully, that makes sense. Let me know what you think.

I'll leave this issue open for now....

bminer commented 12 years ago

BTW... The pause() method is admittedly a bit of a hack... but it works pretty well. All you do is add it right before your async operation, and... then resume() again when your asynchronous operation completes. Or, actually, if you are calling rollback() or commit(), resume() is called for you. Especially if your async operation executes quickly (i.e. a quick call to Redis), the extra pause doesn't cause much of an issue.

The problem is that without pause() mysql-queues has no way of knowing that you are about to do something asynchronous in a query callback. When all callbacks have completed, mysql-queues has no choice but to return control back to the main queue.

The only other way to do it is to get rid of pause() / resume(), and force the programmer to call something like done() after all work is done in the query callback. That would tell mysql-queues to return control back to the main queue. But what if you forget to call done()... part of your program could "hang" because no more queries are getting executed anymore.... they all get queued.... until you run out of memory.

bminer commented 12 years ago

Also, to be clear, if you do not need to decide to commit() or rollback() inside of your async operation, then you don't have to worry about calling pause(), you can just go on your merry way.

For example:

var trans = db.startTransaction();
trans.query("UPDATE", function(err, info) {
    trans.commit();
    setTimeout(function() {
        console.log("No problems here");
        //but don't try calling trans.commit() here... because it's too late.
        //well... unless you call trans.pause() right before the setTimeout function call.
    }, 1000);
}).execute();
bminer commented 12 years ago

Sorry I keep hitting the Close issue button... GitHub should really fix that

bminer commented 12 years ago

@ronkorving - I think that I will close this issue for now. I am very open to any ideas to improve this lib, so please feel free to send me a message or post here if you have any other thoughts about async calls inside of a query callback. Thanks!

ronkorving commented 12 years ago

I haven't had time to respond the past few days, and I feel like I should dive a bit deeper into your library. But I will get back to you on this.

bminer commented 12 years ago

Just wanted to follow up. Any more thoughts on this?

ronkorving commented 12 years ago

Wow, has it been 2 months already? I have to say, in the projects I'm working on, we've collectively decided to move away from MySQL, so I have spent most my time dealing with alternative database technologies.

In retrospect, if you really need an auto-something in order to not have to use pause/resume, I guess I would be in favor of auto-rollback.

bminer commented 12 years ago

Yes... it has been 2 months... unbelievable how the time flies....

By the way, thanks for your suggestion. I initially like the idea of switching to auto-rollback, but I have to play devil's advocate here...

I think the reason why I went with auto-commit from the start was to be a bit more forgiving to people that forgot to call trans.commit(). But, I suppose that if bugs occur in someone's code, the auto-commit "feature" might end up causing data corruption. Then again, an auto-rollback could cause the same kind of data corruption. Either way, you still get a warning message and the possibility for data corruption.

So, I wondered... what does MySQL do? MySQL prefers to auto-commit transactions, actually. From the MySQL documentation: "Beginning a transaction causes any pending transaction to be committed." see here and here.

So, given all of that, I think it's just easier for me to do nothing and leave the auto-commit behavior, as is.

Glad to hear that you're staying away from MySQL land. I'd like to migrate away from it eventually, as well. Just trying to find something fast like Redis, but with some more "guaranteed" persistence. For example, with e-commerce sites, if the power goes out between charging a customer's credit card and the database writing to disk, you could have problems.

OK... I'm ranting. Thx again for the suggestion. Lemme know if you have any other thoughts on the issue.

ronkorving commented 12 years ago

Responding to your rant ;) We're slowly moving to Couchbase, which offers high write performance and better persistence than Redis. Also, it will soon get map/reduce views.