Swallowing errors

[confuser]

https://github.com/apigee-127/swagger-tools/blob/master/index.js#L79-L89

    if (err) {
      if (process.env.NODE_ENV === 'test') {
        // This is not an official mechanism and is done this way only to support our tests.  The reason this is
        // required is because JsonRefs uses Promises and this causes some issues with how our tests were written.
        return callback(err);
      } else {
        helpers.printValidationResults(spec.version, rlOrSO, resources, results, true);

        process.exit(helpers.getErrorCount(results) > 0 ? 1 : 0);
      }
    }

Using a127-magic which depends on this module. Having a problem with any errors being thrown within it's init function are silently being swallowed. This has made tracking down causes for crashes next to impossible, as the process simply silently exits.

The above code is partly the cause, the fact that the error is only ever sent back within the callback under a 'test environment, otherwise the process just exits without any mention of err.

This is also incorrect behaviour as the default node callback styles are callback(error, result), however `initializeMiddleware does not follow this standard pattern, which means that also will need to be changed.

The other cause of this is json-refs not correctly passing back the error to the callback. I've raised a separate issue for this over at whitlockjc/json-refs#58 (comment) alongside a fix

I will open a separate issue on https://github.com/apigee-127/magic with a request to use a correct node style callback.

[whitlockjc]

There are a few minor issues with your statements but all in all, you are right as swagger-tools is not using json-refs properly.

The first issue is that the json-refs used by swagger-tools is very old and has a few bugs, like not bubbling upstream errors properly. This was a result of json-refs trying to support callback and Promise based invocation. This is no longer the case in json-refs Upgrading json-refs would fix the issue.

The second issue is that while initializeMiddleware does not follow the "error-first callback" typically seen in Node.js is a design feature believe it or not. The reason you see the error being returned by initializeMiddleware only in testing is because we have tests that validate the output of upstream errors and do not want the process to stop. It was never intended that initializeMiddleware would be an error-first callback as we always want to print the errors and kill the process. Whether or not it's right doesn't matter now since what's done is done.

So to summarize, we need to update json-refs in swagger-tools and use the Promise-based API properly to avoid this.

[whitlockjc]

I've figured this out. I bet your NODE_ENV=test and that is special cased in swagger-tools. Basically, in initializeMiddleware we will always print the upstream error and exit the process. What's happening here (I think) is that your NODE_ENV is test and so our logic that is intended to be only for swagger-tools tests is being invoked. I'll get it fixed.

[confuser]

Except I get the exact same result locally without even specifying a NODE_ENV, or specifying NODE_ENV=production.

The problem is you're trying to handle a swagger error, but this is a pure JavaScript stacktrace error that is simply being ignored within the quoted call as shown above.

[whitlockjc]

Then that is a json-refs issue. So there are two issues, both of which are described in this:

1. Upgrade json-refs because the one swagger-tools is using is broken
2. For safety, stop keying off of NODE_ENV=test just to catch swagger-tools' tests

I'm on it.

[confuser]

That doesn't cover my use-case entirely.

Your solution is still to force exit my application, rather than letting userland code (i.e. mine) handle the error itself. What if I want to send the error over to a separate logging system first? Even if you did let the exception bubble up, I still couldn't even handle that within a process.on('uncaughtException') because you are forcefully exiting the process.

[whitlockjc]

That use case was never mentioned. I realize initializeMiddleware is not a true "error-first" callback but that was never its intention and it was done this way on purpose. Since there is no graceful way to handle an upstream error in initializeMiddleware, it just never made sense to me because all of swagger-tools' middleware is written for valid Swagger documents. Allowing user land to handle the error would allow you to also continue server startup which is something I do not plan to support. In fact, it's been a long standing feature that servers do not start when the document is invalid.

I don't mind supporting this but next time, how about we start with the "I would like to log the errors encountered in initializeMiddleware" instead of beating around the bush.

[whitlockjc]

The biggest problem we'll run into with supporting this is that it breaks initializeMiddleware. This means it cannot be fixed until 1.0.0 comes out. Upgrading json-refs to fix it can happen now but making initializeMiddleware an actual "error-first callback" cannot.

Let me think about this a little more.

[whitlockjc]

Can you run your server with debugging on, DEBUG=swagger-tools* node ., and let me know if the error is logged at all? Truth be told, I have a problem seeing what is going on. I realize that there would be an uncaught exception if Promises weren't involved and you'd have a stack trace and all. With Promises, there is no way to get that back so I can log it and I think I'm already doing that. If not, I'll fix it.

For 1.0.0, I can make initializeMiddleware take an error-first callback to make it where you can get the error yourself but that won't help with trapping errors throw in middleware, like #331 was running into. You'd have to wrap the middleware invocations in a try/catch to do that.

I'm just looking for a little more information so I fix this the right way.

[whitlockjc]

swagger-tools@0.9.12 is released with enhancements related to this. While we still only pass an Error to the callback during testing, we now will log all errors that happen during initialization even if you're not using DEBUG.

[confuser]

Thanks for the work on this @whitlockjc Unfortunately this became quite a blocker on our end, so I forked and made the changes necessary for us to see what was causing our crashes by passing the error back to magic, and then userland.

confuser@a71e317
confuser/magic@eca3755

However, whilst this does the job and suits our needs, it gave a really bizarre path in terms of how errors are handled and is certainly not a correct solution/fix by any means.

a127.init(function (error, config) {
    if (error) return cb(error)

    // Something that may throw here, e.g. throw new Error('Test')
})

In the above test case, one would normally expect the application to simply crash with a stack trace of the Test error. What occurs is the error argument from the init callback is equal to Error(Test) which is then bubbled up via cb.

[whitlockjc]

So, there are two places where errors can be thrown:

• Tooling: This would be a bug in the tooling
• User land: This would be a bug in your code

At runtime, all of this works fine because of the way the server works. An error thrown in a handler gets handled, etc. Where things are problematic are when you'er initializing the middleware typically: Invalid JavaScript in handlers, errors thrown in init, etc. The commit I made yesterday should fix this. While it does not return the error to user land so you can recover, it does log the error to the console prior to exiting the process.

To fix this officially, I would need to make it so that initializeMiddleware does not do any Promise based work or if it does, it returns the Promise to you so you can have your .catch in there to handle runtime errors that were not handled.