Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-fatal error handling #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mrtcode
Copy link
Member

@mrtcode mrtcode commented Jul 4, 2017

This is a possible solution to error handling in a non-fatal way.

  • Now it never closes, but maybe on some errors it should?
  • Error handling should be implemented to be convenient for client. Now it just sends an error event. Is it convenient?
  • Tests that check socket close codes need do be updated accordingly.

utils.js Outdated
msg = "Error";
}
ws.close(code, msg);
ws.send(JSON.stringify({event: 'error', code: code, message: msg}));
Copy link
Member

@dstillman dstillman Jul 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, code, message: (can always omit matching variable names now)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so write the new code in ES6 everywhere.

@mrtcode
Copy link
Member Author

mrtcode commented Jul 5, 2017

This checks for requestID for all client messages going through handleClientMessage, currently only createSubscriptions and deleteSubscriptions. If requestID exists, it's returned back with the response. Doesn't matter if it's an error response or success, all of them must return the requestID. And there always should be one response.

There are 3 exceptional errors that don't return requestID, because can't parse/validate:

  • Message not provided
  • Error parsing JSON
  • Invalid requestID provided

Let's call those implementation errors. If a client is properly coded those errors shouldn't happen. Unless the data is corrupted while transferred, which should be fatal anyway. In those cases it would make sense to close the connection. Maybe that's the answer to the question which errors should be fatal and which not.

UPDATE:
Actually 4 errors. There is a message length check.

@mrtcode
Copy link
Member Author

mrtcode commented Jul 5, 2017

But actually 'action' not provided and Invalid action are also implementation (or data corruption) errors.

@mrtcode
Copy link
Member Author

mrtcode commented Jul 13, 2017

Removed requestID commit. Arkivo no longer needs this. If we will need this for other things we could return back this feature in future.

So what it does now is just instead of closing connection, it sends the error.

Also fixes bug which exposes 500 error even for non dev site.

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

Successfully merging this pull request may close these issues.

2 participants