kh90909 / OakTerm

Serial terminal for the Digistump Oak over the Particle Cloud
8 stars 2 forks source link

Implement proper error handling and display #64

Closed kh90909 closed 8 years ago

kh90909 commented 8 years ago

Need to gracefully handle Particle API call errors in the promise chains, communicate them to the user, and decide what to do next.

Some possibilities for that 'what to do next':

See @434a90f for a possible framework for putting error handlers into the promise chains.

emcniece commented 8 years ago

Growl notifications seem appropriate for this type of alert

kh90909 commented 8 years ago

Sounds good, but if we get a string of consecutive errors, we might want to suspend everything and pop up a modal until the user responds, so that we're not spewing bad requests, potentially for a long time.

emcniece commented 8 years ago

Makes sense. Something to watch out for will be having retries build up and overlap with other events. Will it be possible to create a backed-up queue, and what happens when many things resolve at once?

To reduce complexity for now, it might be worth leaving the growl notifications as a wishlist item and simply dump events to the terminal. The modal is already present and will suit the task.

emcniece commented 8 years ago

Actually, is there a reason that we shouldn't print the errors in the terminal?

kh90909 commented 8 years ago

My inclination would be to keep the terminal for communication to/from the Oak as much as possible, separate from errors between the user and the Particle Cloud, but I don't have a problem with printing errors to the terminal for now.

However, I've been going through the error cases (see next post) and it looks like in more than half of the situations (e.g. cannot get device info, failure subscribing to events, etc), we won't be able to proceed and will have to pause everything and pop up a modal. In these cases, there's probably not much point in printing an error to the terminal as well.

kh90909 commented 8 years ago

Possible sources of asynchronous errors that need handling

Except for the file read in send_file(), all of these are ParticleJS API calls.

Login
-> get_devices (FATAL)
 -> get_devinfo (FATAL) 
  -> get_variables (MINOR)
   -> subscribe_events (FATAL)

#deviceIDs onchange
-> get_devinfo (FATAL) 
 -> get_variables (MINOR)
  -> subscribe_events (FATAL)

Refresh button and pollers
-> get_devices (FATAL)
-> get_devinfo (FATAL)
 -> get_variables (MINOR)

Send event button
-> publishEvent (NON-FATAL)

send_data (Initiated on "Send Data" button click)
-> publishEvent (NON-FATAL)

send_file (Initiated on "Send File" button click)
-> reader.onerror (NON-FATAL, but need to abort send)
-> send_data (publishEvent) (NON-FATAL, but need to abort send) 

rename_device
-> particle.renameDevice (NON-FATAL)
Error category Action
FATAL ERROR Cannot proceeed, must popup modal
NON-FATAL ERROR Can proceed, but should print error to terminal or growl
MINOR ERROR No significant impact, handle silently unless we get several consecutively

Optional: retry the operation a certain number of times before throwing a fatal error.

The error modal should:

emcniece commented 8 years ago

Excellent summary!

While get_devices is certainly fatal, I have personally seen get_devinfo fail while the surrounding and active queries are working. It was only once - if it persisted then certainly this would be a fatal condition, but since the other calls were working I am hesitant to call it fatal.

Do you think that get_devinfo would be suitable for a MINOR instead?

kh90909 commented 8 years ago

I think it depends which instance of get_devinfo we're talking about. If it fails on login or #deviceids onchange, then it's definitely fatal, whereas if it's from a poller or on clicking refresh, then it could be minor.

I'm inclined to handle this through retries rather than silently ignoring, because we don't know the variables table is valid any more if get_devinfo failed.

Another alternative would be to look at the error code, since it might give us a hint whether the condition is going to be fatal or not.

kh90909 commented 8 years ago

In progress.

kh90909 commented 8 years ago

This is pretty much finished in the error-handling-wip branch (rawgit link), including promise-based retries and a modal for fatal errors. It's handled all the error cases I've thrown at it so far.

With retries and error handling, the promise chains get pretty complicated and tricky to follow and debug. Maybe there are some simplifications that can be made.

Will squash this into a single commit and put it into a PR for testing.