Exceptions in handlers getting swallowed up

[vthunder]

This took me a while to figure out, I'm not sure where, but exceptions in (at least the success) handlers are being caught and not rethrown. Code like this:

// db is a configured dynamodb client object
db.client.query(query)
.on('error', db.onError)
.on('success', function(res) {
console.log("hi there");
throw "fun times";
}).send();

results in "hi there", but nothing else on the console, even if you set up a domain or an uncaught exception handler to print them out.

So most likely there's a try/catch somewhere that should rethrow (my guess).

[lsegal]

Unfortunately it's not possible to throw exceptions from asynchronous callbacks in JavaScript. We are aware of the possibility of losing errors from inside callbacks, and we were experimenting with wrapping the callbacks in try/catches that would log these lost exceptions to stderr. I think this would be useful for debugging purposes, but this error log will not stop program flow, so it too can get lost in your output. Also, a library like aws-sdk should probably not try to take over your stderr pipe, so we would at least have to make this toggleable.

As you allude to in #75, domains are one of node's proposed solution to this overarching technical limitation of the language. Domains are also one way we would like to support this kind of a problem, but the API is not stable enough for us to support fully yet-- more in that issue.

[cjhanks]

Can you point to the section of code which is swallowing exceptions so I can disable it? The inability to see where errors propagate from is making development very difficult (think typos or reference errors).

Note for others having this issue logging the err object in event_emitter.js:126 makes debugging at least possible.

[lsegal]

@cjhanks the latest commit (above) should make it so exceptions now properly propagate out from the callback. You can now also use domains to manage these exceptions (see #75).

[vthunder]

Thanks a ton for fixing this - being able to catch exceptions at least at the run loop will make it much easier to debug issues.

[bigeasy]

Where does this stand. Exceptions are still being swallowed as far as I can tell. It would be nice, for the little utilities that I'm writing, to just allow a thrown exception propagate up and kill the program.

var AWS = require('aws-sdk')

AWS.config.loadFromPath(process.env.HOME + '/.aws')
AWS.config.update({region: 'us-east-1'})

new AWS.EC2().describeInstances(function () { throw new Error })

The above runs without writing any output.

[tomcollins]

I am also seeing the behaviour described in the previous comment. Was this really fixed?

[lsegal]

Unfortunately this change had to be reverted due to #131, since this original change caused requests to be retried when errors were thrown from callbacks; not what you would want to happen. I'm currently looking into a way to get the best of both worlds.

[bigeasy]

Heres some work that made me think of this issue.

bigeasy/strata@87fccc1
bigeasy/strata@2912e87

[joonas-fi]

This bit me as well. I had if (err) { throw err; } in my callback, and I'm using process.on('uncaughtException') to log exception and end process.

I think this is really surprising behaviour and this should be fixed. It is outrageous for a library to just swallow exceptions and hide them.

[mhart]

Errors have been problematic for a while FWIW: See #4 and #307 (the discussions there may be germane to this issue)

[lsegal]

@joonas-fi just to update-- this issue is fairly old. We actually did end up re-introducing logic to throw errors out of the callback, first in 3cdd424 (which is shown in this issue above) and then in a more elegant way with the release of v2.0.5 as @mhart pointed at in #307.

In short, if you're on the latest version of the SDK your errors should be propagating out of the callback. If this is still happening to you, can you verify and report the version of the SDK you are using?

[joonas-fi]

@lsegal:

├─┬ aws-sdk@2.0.17
│ ├── aws-sdk-apis@3.1.8
│ ├─┬ xml2js@0.2.6
│ │ └── sax@0.4.2
│ └── xmlbuilder@0.4.2

Fresh install from npm a few days ago.

I'm using s3.putObject()

(edited:)

$ node --version
v0.10.32

[lsegal]

@joonas-fi do you have an example of code that swallows an error?

[joonas-fi]

This is somewhat what I'm doing:

var aws = require('aws-sdk');
var fs = require('fs');

// set AWS credentials here...

process.on('uncaughtException', function (err){
    console.log('uncaughtException:', err);

    // re-throw: exits process
    throw err;
});

var s3 = new aws.S3();

s3.putObject({
    Bucket: 'my-bucket',
    ACL: 'INVALID-ACL', // this should trigger error since its value is invalid
    Key: 'foo.txt',
    Body: fs.createReadStream('/tmp/foo.txt')
}, function (err, data) {
    if (err) {
        console.log('throwing; not reaching process.on("uncaughtException")', err);
        throw err;
    }

    console.log('operation was ok - should not happen');
});

And this is the output:

throwing; not reaching process.on("uncaughtException") { [InvalidArgument: null]
  message: null,
  code: 'InvalidArgument',
  time: Sun Oct 19 2014 10:14:10 GMT+0000 (UTC),
  statusCode: 400,
  retryable: false }

[lsegal]

Ah, I can explain this. Errors will propagate, except if it happens to be the same error object the SDK gave you in the callback. Not saying that's how it should work, just that it's how it works right now:

https://github.com/aws/aws-sdk-js/blob/master/lib/request.js#L29

For what it's worth, that check does seem a little odd. I just ran our tests with that e !== err check removed and everything passes, so we might be able to just rip that out and have things work normally. I'll do some more tests first to make sure nothing is relying on that behavior.

That said, the following does throw, which you could use in the meantime:

s3.putObject(params, function(err) { throw new Error(err) });

[joonas-fi]

Oh okay, I kind of understand. Still that does not make much sense, since surprising behaviour is always surprising.

As a user I wouldn't expect things to get handled differently if I just throw the original error object out, instead of throwing "custom" error object out.

Good to know all tests still pass when the check is ripped out, sounds promising.

Anyways, thanks for helping with this. I will use your trick of throw new Error(err) for now. I hope this gets fixed because I do think many people get bit by this.

[bigeasy]

Why not do this inside aws-sdk?

try {
    callback(error)
} catch (e) {
    AWS._thrownByUser = e
    throw e
}

Later:

try {
    AWS._somethingThatWillCallTheBlockAbove()
} catch (e) {
    if (e === AWS._thrownByUser) {
        throw e
    }
    AWS._handleError(e)
}

[lsegal]

@bigeasy Storing the "last error" on the global object could potentially cause a memory leak if it wasn't properly cleaned up-- especially if at some point in the future we start referencing the related request object information in the error. More importantly, it's likely to cause async-based race conditions if a user launches two requests at once, or even does something in the callback that causes another AWS error to throw.

I think the right solution here is to just drop that e !== err check if we can. I'll have to investigate why that code was added in the first place, but it seems like we could refactor that logic out.

Thanks for your feedback!

[bigeasy]

@lsegal None of that that you said would happen would happen. That's not how JavaScript works.

[lsegal]

@bigeasy there would certainly be an issue with memory leaks if the reference was not properly cleaned up. As for the race condition issue, perhaps we're using different terminology-- the issue here is that multiple sync / async operations writing to the same object can cause one of the callbacks to read the property too late. It depends on how this is all implemented, obviously, but the initially proposed solution above seems like it would be less error prone.

Can you elaborate more on where exactly those two blocks would be in the SDK? Perhaps I'm not following on some of the details.

[bigeasy]

There would be no async race condition because try/catch is synchronous. There is no way to asynchronously continue the unwinding of the stack. The reference is set after the callback, not before.

The leak is a non issue. You would only ever leak one exception. That would get all cleaned up when the exception unwound the stack to all the way to the event loop and the process crashed.

If the program continued, that would exceptional and incorrect. If it did continue, you could simply set the reference to null the next time you called the user back.

function callUserBack (callback, error, result) {
    AWS._thrownByUser = null
    if (callback) {
        try {
            callback(error, result)
        } catch (error) {
            AWS._thrownByUser = error
            throw error
        }
    }
}

But, again, recovering from an exception thrown through async calls is wrong.

[mhart]

@bigeasy I'm not quite sure why you need a global property set instead of just setting it on the error (ie, e._thrownByUser = true ... if (e._thrownByUser)?

In any case, I'm in agreement that the way aws-sdk handles exceptions is troublesome – but I've made my case on this before.

[bigeasy]

@mhart Because I'd rather not alter an exception thrown by the user in any way. That's just me. It's not like a package scoped property (not global) it is any less safe or correct.

[lsegal]

Opened issue #392 to track this, @mhart, @bigeasy, @joonas-fi. Thanks for reporting and all the suggestions, they're all very helpful!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.