-
-
Notifications
You must be signed in to change notification settings - Fork 829
Check for liveliness on submission when the server was previously dead #3218
Conversation
Fixes element-hq/element-web#10017 Specifically the `return` at the end of the diff fixes the problem, but it seems worthwhile to check for liveliness when we know the server has been dead in previous attempts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just 1 thing to look at.
busy: false, | ||
...AutoDiscoveryUtils.authComponentStateForError(e), | ||
}); | ||
if (this.state.serverErrorIsFatal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume serverErrorIsFatal
comes from authComponentStateForError
above. setState
may not immediately modify this.state
so keeping the result from authComponentStateForError
in a local var to read it out might be good idea here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when is setState not immediate? We rely on this behaviour in quite a few places in the app...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://reactjs.org/docs/react-component.html#setstate:
Think of
setState()
as a request rather than an immediate command to update the component. For better perceived performance, React may delay it, and then update several components in a single pass. React does not guarantee that the state changes are applied immediately.
AFAICT, we use the (second) callback argument of setState
often to be sure the state is set. No need for this here though, as you could keep the data in a local variable to query it independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair, I wonder how much of our app would break when/if they change that assumption of ours...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know of any places off the top of your head where we assume this.state
to be updated immediately? We should probably fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember exact locations, but I believe places in MatrixChat and various atuh components at the least.
When we rewrite it in Rust we'll get 'em.
Fixes element-hq/element-web#10017
Specifically the
return
at the end of the diff fixes the problem, but it seems worthwhile to check for liveliness when we know the server has been dead in previous attempts.