Some API usage errors/scenarios result in silent failure #42

Open
magwo opened this Issue May 29, 2012 · 4 comments

Comments

Projects
None yet
2 participants

magwo commented May 29, 2012

There is some problematic error handling code in this library, when you are passing incorrect parameters and so on into certain functions.

For example:

if ( ! fileHandler.options) {
  return;
}

This results in a silent "hang" in naively-written code, due to the callback not being called. Why not simply call the callback and pass an error parameter? This would make it much easier for developers to discover and debug incorrect usage of the API. Or at least throw an exception or something. A silent failure by simply returning is not acceptable.

This style of error handling (or lack of) discouraged me from switching to this library from our own knox fork. Otherwise it looks like a nice and well-composed library.

Owner

SaltwaterC commented May 29, 2012

Can you provide certain examples? That piece of code is taken out of its context.

fileHandler.options = internals.filterByteRange(fileHandler.options, false, callback);
if ( ! fileHandler.options) {
    return;
}

filterByteRange() returns false in case of failure, while the callback receives an Error instance as the first argument which is the node.js convention. The implementation is available here. In this case I could return the Error instance instead of passing it to the callback, but then the handling would look like:

fileHandler.options = internals.filterByteRange(fileHandler.options, false);
if (fileHandler.options instanceof Error) {
  callback(fileHandler.options);
  return;
}

Doesn't improve anything though.

magwo commented May 29, 2012

Ok, for example with putFile I got a request hang due to not providing a valid headers object it seemed (I passed null):

        config.putFile = function (path, file, acl, headers, callback) {
            file = p.resolve(file);
            headers = internals.normalizeHeaders(headers);
            headers = internals.checkAcl(acl, headers, callback);
            if ( ! headers) {
                return;
            }

Perhaps I am using the library in an incorrect way, but it still shouldn't fail silently. I think your proposed structure of always calling the callback in the event of a critical failure is good. As an API user I want to be guaranteed to always have my error/success callback be called whatever the failure may be.

If the previous code in your example is guaranteed to have called the callback in the case of an error, then I don't see the point of the if statement and silent return -- I would much rather have the code continue executing and fail dramatically.

Owner

SaltwaterC commented May 29, 2012

I get it now. I'll implement those pieces of error handling in v0.7 (currently the experimental branch). I have plans for heavy refactoring anyway. Unfortunately I spent most of my time dealing with service side error handling, but I didn't take care of some cases where users would pass null for an argument where a key - value object is expected.

magwo commented May 29, 2012

Cool! Good job on many parts of the lib, especially the docs.

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