Return undefined for error on success

[aerospike]

Return undefined for error on success

[vivekkrbajpai]

Totally agreed the way it handles error is very bad.In some cases it crashes the entire application. With error message segmentation fault (core dump )

[AlgoTrader]

After dozens of core dumps with their C client and very poor support of scripting langs I am not considering AS as a serious production-ready solution. I am also not happy of how they communicate to developers, so, I put it off.

[cstivers78]

I agree it is idiomatic to not return the error on a success. We plan on changing it, but we have to weigh the impact on existing users, which I should have mentioned earlier, but I had let it slip. Apologies.

@vivekkrbajpai It is my first time hearing about core dumps associated with error objects. Can you file another ticket so we can follow up on that issue?

As for other issues, we work diligently to resolve them. We hope you are patient enough to wait for a resolution or assist us by indicating the severity of the issue with regards to your product, so we can prioritize accordingly.

[vivekkrbajpai]

Thanks for the response @cstivers78 👍 .I will create the new issue with my logs.

[ctavan]

👍 from my side for passing null as a first callback argument if the operation was successful, this was confusing me right away when I started to work with the node driver.

But if you do the change, please stick to the node conventions and pass in null as a first argument if the operation was successful (as opposed to undefined which is mentioned in the title of this issue). It is common convention to pass null, not undefined, see e.g. http://fredkschott.com/post/2014/03/understanding-error-first-callbacks-in-node-js/

[AlgoTrader]

undefined and null for error are both valid. The difference is subtle.
callback() is success with undefined
callback(null) is also success but with null
They live in their own world, I do not think they care of node.js style and conventions. They just mechanically ported C client to node

[ctavan]

@AlgoTrader a matter of taste I suppose. I think it is good node.js style to explicitly state the success case calling callback(null); ;)... But as I said, it's probably a matter of taste. Anything falsy should usually do since the canonical way of error handling is usually:

asyncStuff(function(err, result) {
  if (err) {
    console.error(err);
    return;
  }

  // do stuff with result
});

[AlgoTrader]

You are right, matter of taste. I use null but for third party code I also expect undefined

[rschmukler]

Just chiming in that this is very much an antipattern within node and that for things that assume falsy values (like libraries that convert callbacks to thunks/promises) it breaks them. +1 for changing it. No opinion on null vs undefined (null does feel more "node") but it should definitely not be an object on success.

[EmilTholin]

+1 Aerospike is phenomenal, but this issue makes the Node driver very unpleasant to work with. It breaks the convention, and it makes it hard to wrap with bluebird. Maybe you could include a flag when initializing the client, if not changing it altogether?

var client = aerospike.client({
    hosts: [ { addr: '127.0.0.1', port: 3000 } ],
    standardNodeError: true // error === undefined upon success
});

[eljefedelrodeodeljefe]

This currently really stands in my way to really dive into aerospike 100% will a PR be quickly welcomed? @cstivers78

There is a similar issue with (the user incorrectly) passing, taht floats breaking the client or similar.

[jhecking]

@eljefedelrodeodeljefe, a pull request would be welcome! As Chris had pointed out, this is a backwards incompatible change and can potentially affect existing application code. So we need to be mindful of this. Maybe you can open a PR and quickly describe the changes you plan to do? I'd be happy to work with you on this.

[eljefedelrodeodeljefe]

@jhecking moved this to #105

[jhecking]

For v2 of the Aerospike Node.js client we have changed the semantics of the callbacks done by the client to follow the Node.js error-first callback semantics where the error argument is null if the operation was successful. v2 is now available as a pre-release in version 2.0.0-alpha.1 on npmjs.com.

Some additional resources:

• Pre-release announcement
• Changelog
• List of backward incompatible changes