Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Allow changing servers on nonfatal errors #3102

Merged
merged 2 commits into from
Jun 13, 2019

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Jun 13, 2019

@turt2live: hopefully this is a sensible way to do this

Fixes element-hq/element-web#10016

@dbkr dbkr requested a review from a team June 13, 2019 17:24
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

this works, however so does:

try {
  await AutoDiscoveryUtils.validateServerConfigWithStaticUrls(hsUrl, isUrl);
} catch (e) {
  if (AutoDiscoveryUtils.isLivelinessError(e)) {
    // Continue anyways logic
  }
  // else E_BROKEN
}

lgtm either way. Would be nice to have 1 less blank line on line 121 though.

@dbkr
Copy link
Member Author

dbkr commented Jun 13, 2019

I thought we needed the return value from validateServerConfigWithStaticUrls to continue here though? Which we don't get if it throws.

@turt2live
Copy link
Member

we do, but we can always call it again with syntaxOnly=true.

@dbkr
Copy link
Member Author

dbkr commented Jun 13, 2019

In this case we need to do so immediately though because we need to pass it to this.props.onServerConfigChange? I guess actually this is equivalent to just specifying syntaxOnly in the first place though?

@dbkr
Copy link
Member Author

dbkr commented Jun 13, 2019

oh - it's not equivalent because nonfatal error !== liveness check fail (ie. we still want to fail if the HS isn't reachable)

@turt2live
Copy link
Member

ah, yes, it would be... so just a try/catch around a syntaxOnly call and you should be fine

@turt2live
Copy link
Member

hmm... if non-fatal is != to liveliness then the code you have here is fine, because syntaxOnly=true disables liveliness

@dbkr
Copy link
Member Author

dbkr commented Jun 13, 2019

ok yeah, I think this is right then, ie. it will let you continue with the change if the IS server is down but not if the HS is down.

@dbkr dbkr merged commit 2a7301f into develop Jun 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If the IS is down, you can't change your HS from the default
2 participants