Remove try_callback

[BridgeAR]

I can't see that this would harm anyone

[bcoe]

@brycebaril and/or @mranney, could you give some context regarding the try_callback method. Is this change going to break domains?

[BridgeAR]

Reference #543

[BridgeAR]

The original commit for this is 202df58 and it had nothing to do with domains. As I see it the only thing with domains was, that they had to be added added in the try_callback as node-redis catched those errors too. By removing this nothing should be broken at all.

[BridgeAR]

The original commit is faulty and this should have never been introduced. It tries to fix wrong user code by throwing the user error async instead of sync.

So this is a bug fix overall without any negative impact on domains.

[brycebaril]

Judging by the original PR, the reason this exists is for multiplexed replies such as:

-Error message\r\n+OK\r\n

Rethrowing on the next stack allows the parser to continue and handle the +OK\r\n message.

[BridgeAR]

But those do not happen in the user callback?

[brycebaril]

@BridgeAR they happen when there is an error in a multiplexed reply, which are emit by the parser synchronously https://github.com/NodeRedis/node_redis/blob/master/lib/parser/javascript.js#L174-L180

particularly the rethrow on https://github.com/NodeRedis/node_redis/blob/master/index.js#L539-L547 is designed to protect this

[othiym23]

I think that, worst case, this will require people who want calls into redis to play nice with domains will need to go back to .bind()ing their callbacks before handing them off to the driver, just like they would with continuation-local-storage or anything else that needs to deal with the way Redis bundles callbacks together for multiplexing.

I'm +0 on this change – I think the way that this code deals with errors is sufficiently opaque that relatively few users of the library understand how to handle errors with it correctly, and this change is likely to leave people in a confused state in at least a few cases, but the patch also removes some complexity.

[BridgeAR]

@othiym23 the binding is still in place and working. Your original test is passing just fine.

@brycebaril I wrote different multiplexed examples now and they are all handled properly and just the same as with the try catch. There was an error that the js parser replied errors as strings that I already fixed.

Overall I'm very certain that this code was a bug not a feature ;)

[BridgeAR]

I'm uncertain if we want to support this behavior but this is how the tests behave right now. I'd say it's very unlikely that any user is going to call set / setex without any arguments. They might be undefined, but that would result in an error... I'd say let's not support this?

[BridgeAR]

Ping @bcoe @brycebaril

[BridgeAR]

Since there are still some open issues about undefined and null values passed into redis we're going to have to have a closer look at this later on anyway. Therefor I'll merge it as is.

[BridgeAR]

This is going to fix three things:

1. This is going to fix issues where errors could have taken down user apps by throwing them async into nirvana. Instead, they will be emitted from now on, so the user is going to be able to catch them appropriately. Fixing node_redis: no callback to send error: ERR syntax error #755
2. This is also fixing issues with users making errors in their callback and not catching them properly.
3. Using set with an undefined or null value and no callback is going to emit an error from now on. This is to make the behavior of using a callback / no callback consistent.

As a nice side effect this should not only make the code base clearer but also speed up some calls as the try catch is not optimized well in v8.

This is not breaking the domains as I already tried to outline. That is still in place and working as it should.

This might break users code that tried to work around these issues but they should still be fixed in my opinion. So this should go into the next major version.

[BridgeAR]

I edited the comment above and removed the error wrapping into another PR

[bcoe]

@BridgeAR I like that this simplifies the error handling logic, moving towards always emitting, and that it gets rid of some opaque code. I say that we release this as part of v2.0.0, and let the community point us towards any issues.