[appveyor] Error handling in BaseService

[paulmelnikow]

This is some work in progress for error handling in BaseService. It works, though it's not tested yet. I'm not even sure I like all of it.

The part I really like is a clear distinction between programmer errors ("internal errors") and runtime errors, and the ability to configure the program to let the programmer errors bubble up in development and unit testing. This saves a huge amount of time because it generates ordinary stack traces when things go wrong. And, if these errors occur in production, we'll catch them, and display shields | internal error which is the equivalent of a 500 error.

@chris48s You and I are both eagerly awaiting this feature. Curious to get your thoughts and input on this. Would you like to to take a stab at reworking the helper methods? Feel free to push some commits to this branch.

[shields-ci]

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS

[chris48s]

Should this be throw new InvalidResponse() here?

[paulmelnikow]

Indeed! Fixed.

[chris48s]

I know this is already a good reduction in boilerplate, but I feel like we're very often going to chain .then(checkErrorResponse.asPromise({})) here. Do you think it would be worth maybe wrapping this in the base class so that the error handling function is passed as a param with a default value?

.. something like:

  async _wrappedRequest(url, options, handler=checkErrorResponse.asPromise({})) {
    return this._sendAndCacheRequest(url, options).then(handler);
  }

in base.js and then we can call const json = await this._wrappedRequest(url, options).then(asJson); when we want to use the default handler (which is probably ~90% of cases) and const json = await this._wrappedRequest(url, options, myCustomHander).then(asJson); in the case where we want to customise the message or deal with a non-standard error condition or whatever.

If there is a situation where something really non-standard happens, we could always override _wrappedRequest (or whatever we call it - better names welcome) or just call _sendAndCacheRequest directly. Does that seem sensible?

[paulmelnikow]

Agreed, the vast majority of the time we're wanting to do the same thing here. Adding a method is a nice idea. Perhaps it could be _performRequest or _makeRequest.

For a custom not-found message, which is or should be relatively common, perhaps we could use a separate static get notFoundMessage() getter.

As you say, this could be overridden when something more custom is needed.

I'd even go so far as to say that, perhaps, the method should be _requestJson, which parses JSON by default, and the the handful of services which need something else could define or use a different method, or call external helpers (like the SVG badge fetcher). Then the 90% case looks like const json = await this._requestJson(url, options);, including if we're overriding the not-found message, and doesn't need to require any helpers.

I'd prefer to have concerns a little more separated – having JSON-defaulting stuff in BaseService begins to cross the line for me – but we can revisit that later. With a few more examples, probably separating concerns will be a bit easier.

[chris48s]

I'd even go so far as to say that, perhaps, the method should be _requestJson
...
I'd prefer to have concerns a little more separated – having JSON-defaulting stuff in BaseService begins to cross the line for me

I had a very similar thought to this.

Maybe this is another good way to look at this:
Now BaseService is a class we can extend it, so you can declare

class BaseJsonService extends BaseService

and chain .then(asJson) in the BaseJsonService version of whatever we call that method, but not in BaseService. If we spot other common patterns that we are using in lots of places, we could also define other base classes. We don't have to do it all in one object.

[paulmelnikow]

Sure! I'm good with that as a starting point. Though, down the line, I'd also like to consider breaking the vendor-calling code out of the service class.

[chris48s]

I'm really confused about where the 'inaccessible'/connection error gets thrown. It does seem to work if I add a test with .networkOff() but I just can't find where it happens.

[paulmelnikow]

Huh, I've just tried adding a test with .networkOff() but I can't get it to work. Not only that, the problem spills over into subsequent tests.

I wonder if this is a bug in our error handling, as a result of mixing promises and callbacks somewhere. AFAIK the exception raised by .networkOff() bubbles through the err to the request callback which means it should definitely not be unhandled.

Although… our new handler function is an async method. It returns a promise, which means when it errors out, it's a rejected promise. However, the callsite doesn't check if the function returns anything, nor .catch() it. Probably the return value of that function invocation should be checked, and if it's a promise, a catch() should be chained onto it.

[chris48s]

Right sorry. I had overlooked that appveyor.tester.js has tests for the /ci endpoint (which has been migrated to the new architecture) and the /tests endpoint (which is still implemented in server.js). I added a test for the /tests endpoint. No wonder I was getting confused :D If I add a test for the right endpoint I get the same behaviour you've described.

I guess then the comment here is: we should also handle the 'connection error' case in the standard error handling, maintaining as much consistency with NotFound and InvalidResponse as possible (rule of least surprise and all that), accepting the caveat you've mentioned above.

[chris48s]

Question: If we want to throw an exception here in the handle() function because we detect an error condition based on the response content rather than the status code, can you give an example of how to do that? Based on the calling code

     try {
       return await this.handle(namedParams);
     } catch (error) {
       ...
     }

it seems like I should be able to throw new Error('foo'); or return Promise.reject(new Error('foo')); in here, but both of those throw UnhandledPromiseRejectionWarning if I try that. Obviously once we've got a more final implementation we can document stuff like this.

[paulmelnikow]

Right! So, the way to signal an exception based on the response content is to throw new InvalidResponse(message), or just throw new InvalidResponse() to display invalid.

That said, the UnhandledPromiseRejectionWarning is a bug (see #1590 (comment)). The behavior I would expect in that situation is that the thrown error is caught and emitted directly. (I think that's possible.)

[chris48s]

Right yes, so if I add the line throw new InvalidResponse('foo'); here, that does what we expect. If I add throw new Error('foo'); here that throws UnhandledPromiseRejectionWarning. It seems like that should also be caught in invokeHandler()

[chris48s]

Bit more info: that behaviour is only true with HANDLE_INTERNAL_ERRORS=false.

If I set HANDLE_INTERNAL_ERRORS=true there, we get a standard 'internal error', but I guess the error it is catching/re-throwing is UnhandledPromiseRejectionWarning rather than the new Error() we threw.

[paulmelnikow]

It took me a while to get clarity about what needed to happen here, though I think I've gotten it working well now. Would you like to take a look?

[chris48s]

I like the way this just transparently passes through the inputs so it 'chains' nicely

[chris48s]

Pulled down the latest changes and the inaccessible handling code is looking good 👍 , but if I edit the service code to

module.exports = class AppVeyor extends BaseService {
  async handle({repo, branch}) {
    throw new Error('foo');
    ...
  }
};

I am still getting the Unhandled promise rejection error. Is it the intention to try and handle that, or is it intended for those not defined in services/errors to be unhandled in this way? I can see reasons for both..

[paulmelnikow]

That snippet is a great example of code I‘d advocate treating as a bug. In other words, it’s a programmer error.

Our error handling strategy should differentiate the two kinds of errors: programmer errors, and runtime errors. Probably you’re familiar with this distinction, but I’ll explain what I mean for others reading.

A runtime error is when something goes wrong that can be expected, like a service returning bad data, a network connection failing, or a disk having filled up. Of course the disk example isn’t relevant to Shields, but the others are.

A programmer error occurs when the code fails under certain inputs, and the programmer hasn’t correctly guarded themselves. Examples are the built in exceptions like TypeError, ReferenceError, ParseError, which manifest when something has gone wrong that the programmer hasn’t checked for. Of course malformed service data or a bad value for a query parameter can cause a TypeError. However it’s the programmer’s job to adequately validate the data before making assumptions about the types, thus preventing the TypeError from occurring.

Declarative validation using something like Joi probably is best, because validators written this way are terse, easy to read, write, and change, and delegate the hard work and edge cases to a well tested library.

The disadvantage of automatically handling all exceptions is that the programmer errors and legitimate runtime errors are indiscernible. The bugs are masked as runtime errors.

Guarding against error conditions is significantly more work but the payoff in developer velocity far outweighs it. No more editing a catch block to print err.stack: the programmer errors will just manifest when a mistake has been made, whether in a test failure or when running the server locally.

At runtime we want to show the user a message rather than just leave them hanging – hence shields | internal error. It also motivates us to find and fix these bugs so we can display more helpful error messages. And hopefully it motivates contributors to do the same! It’s a way of saying, “you can fix this!”

That’s a long-winded argument on error-handling strategy. I hope it’s convincing.

I’d much rather the programmer error be reported directly, rather than wrapped in an UnhandledPromiseRejection. I’ll work a bit more at that to see if it’s possible.

What do you think?

[chris48s]

What do you think?

Yep - nice. Totally on board with this. In the case of an unexpected error, we want it to bubble to Sentry, not the user 👍 I'd suggest one refinement to this: In order to make this a bit more extendable in future, I'd suggest we modify services/errors.js to define a base class, like:

class ShieldsRuntimeError extends Error {
}

class NotFound extends ShieldsRuntimeError {
...
}

class InvalidResponse extends ShieldsRuntimeError {
...
}

class Inaccessible extends ShieldsRuntimeError {
...
}

and catch any error of class ShieldsRuntimeError. That way if we do encouter a situation other than 'not found', 'invalid' and 'inaccessible' that we want to handle rather than throw the handler is more flexible. There is also a point to bear in mind here to document how to throw errors correctly once we update the docs to reflect the new service architecture.

There is one edge case which occurs to me here in terms of how we handle errors: The dynamic json/xml badges. If you think about an issue like #1493 for example, when a user is "developing" (for want of a better word) a badge using those features there is value in the user being able to see errors in that case so they can check their inputs/refine their query/whatever. In that case, maybe we do want to feed back 'internal' errors to the user instead of bubbling them to sentry. I don't particularly want notifications every time a user is trying to feed HTML into their dynamic JSON badge, but it might help them work out what they are doing wrong if they can see a parseError. If you think about my suggestion above, that might be a case where we want to define

class DynamicBadgeError extends ShieldsRuntimeError {
...
}

and just wrap anything that gets thrown in a DynamicBadgeError, but also interested in your thoughts on that situation.

I’d much rather the programmer error be reported directly, rather than wrapped in an UnhandledPromiseRejection. I’ll work a bit more at that to see if it’s possible.

Yes - it would be nicer developer experience if we could do this. Simultaneously the original error is available in the stacktrace so I don't think this point should hold up merging this feature if it is really hard to do and it is the last outstanding issue.

[chris48s]

There is one edge case which occurs to me here in terms of how we handle errors: The dynamic json/xml badges...

There is possibly some overlap here with @platan 's comment in #1623 (comment)

[paulmelnikow]

I don't particularly want notifications every time a user is trying to feed HTML into their dynamic JSON badge, but it might help them work out what they are doing wrong if they can see a parseError. If you think about my suggestion above, that might be a case where we want to define …

Yea! I just posted in that other thread you linked, and think e.g. invalid json is a good message to return in the case of a dynamic badge's data source being bad. We should also keep in mind that, since data sources go down sometimes, invalid json could also be a runtime error from the developer's perspective too – meaning I'd probably color it gray rather than red.

The other case is where the query fails. I think that could be because the query expression is itself invalid. That's a permanent problem with the request, a 400 error, and should be red.

Another possibility is that the query is valid but matches / returns nothing in the data. That's ambiguous. In other word, it could be a permanent problem with the query, though it could also be a transient problem with the data. In that case we should assume the less severe, and return a gray error. Heh, come to think of it, it might not be an error at all. Aren't we displaying something now, like no data, in that case? Seems like that's the right thing to do.

For debugging purposes we could also make a JSON or version of the badge which returns the full error message. Or, even better… a front-end dev tool for dynamic badges! It could fetch the page and run the query right in the browser! It's not so hard to ship these libraries to the front end.

[chris48s]

Or, even better… a front-end dev tool for dynamic badges! It could fetch the page and run the query right in the browser! It's not so hard to ship these libraries to the front end.

Glad you brought this up. I have also thought before that a 'sandbox' environment for making dynamic badges would be pretty cool. Then you could provide detailed error reporting in the 'sandbox' environment and less descriptive errors in the SVG. Rendering exceptions to SVG is not great UX for either the user attempting to extract meaningful debug info from it, or the end-user who is seeing it in someone's README when something goes wrong.

That said, lets not try and boil the ocean all in one pull request :) I think the key things this identifies are:

• There are things other than 'not found', 'invalid' and 'inaccessible' which we may want to class as a runtime error (at least in some sitautions).
• We may want to provide different levels of error reporting in different output formats. e.g: the SVG render may have a different level of error reporting from a JSON endpoint backing a sandbox environment for 'developing' dynamic badges.

Our mechanism for throwing/handling errors should be flexible enought to accommodate that.

[paulmelnikow]

Great summary!

I don't think this point should hold up merging this feature if it is really hard to do and it is the last outstanding issue.

I may not have a moment in the next couple days to try to tackle this so I think I will merge without it. Once this is merged we can return to that, and also easily tweak the class hierarchy to support more kinds of runtime errors.

[Daniel15]

Does this have to be async? It's not doing anything asynchronously.

[paulmelnikow]

I think if it weren't async, it would not return a promise.