Fault tolerance

[dmnisson]

Links

• Issue: Socket connection issues #400
• Server repo PR: https://github.com/UPchieve/server/pull/351

Description

• Fault tolerance code is changed to monitor connection status in the Vuex store
• A modal dialog is displayed if there is trouble reaching the server to begin a new session
• Session initialization is made fault-tolerant using promise-retry
• A warning message is displayed in the chat for a lost socket connection
• Session data is now stored only in the Vuex store, rather than being copied in the global SessionService.currentSession

Developer self-review checklist

• Potentially confusing code has been explained with comments
• No warnings or errors have been introduced; all known error cases have been handled
• Any appropriate documentation (within the code, README.md, docs, etc) has been updated
• All edge cases have been addressed

[ericyliu]

left some comments! please feel free to message me on slack or respond on here if you have any questions

[ericyliu]

i would generally not handle promise returns and errors nested inside like this (as it can be difficult to follow for future programmers). instead, you could handle this promise outside like:

retryHttp: function() {
  return promiseRetry(retry => {
     ...
  }, {
    retries: FAULT_TOLERANT_HTTP_MAX_RETRIES,
    maxTimeout: FAULT_TOLERANT_HTTP_MAX_RETRY_TIMEOUT
  })
    .then(this._successHandler, this._errorHandler)
    .catch(res => {
      if (res.status === 0) { ... }
      ...
    });
}

[dmnisson]

The catch handler needs to be nested inside the function that is passed to promiseRetry because it needs to call the passed-in retry function to signal to promiseRetry that the function needs to be retried.

I think I'm going to use a different package for managing the retries and backoffs and rewrite this whole chunk of code--the promiseRetry module looked tempting but as I started implementing the things I needed to implement here (like stopping the requests when the user clicks "Abort") my solutions became increasingly ugly.

[ericyliu]

are you creating the retryHttp method and then calling it immediately? unless im readying the brackets wrong, youre doing something like:

return {
   isAborted: false,
   retryHttp: function() { ... },
}.retryHttp();

so why not just directly call whatever retryHttp is doing instead of defining it and then calling it?

[ericyliu]

also, if you want to, we can pair on some of this stuff as i dont know i conveyed myself super clearly and its confusing

[ericyliu]

looks great! thanks for the changes

[ericyliu]

a way to avoid the crazy if statement could be:

if (this.session && this.session._id) {
  if (this.session.student && this.session.student._id === this.user._id) return location.reload();
  if (this.session.volunteer && this.session.volunteer._id === this.user.Id) return location.reload();
  return this.joinSession(this.session._id);
}

i think this looks more readable, but up to you whether you want to change or not

[jkeat]

awesome to get rid of this :)

[jkeat]

great work @dmnisson!