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

Check for liveliness on submission when the server was previously dead #3218

Merged
merged 2 commits into from
Jul 15, 2019
Merged

Check for liveliness on submission when the server was previously dead #3218

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6a18b1
Check for liveliness on submission when the server was previously dead
turt2live Jul 15, 2019
2bc0e8e
Don't rely on React being fast
turt2live Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/components/structures/auth/Login.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,31 @@ module.exports = React.createClass({
return this.state.busy || this.props.busy;
},

onPasswordLogin: function(username, phoneCountry, phoneNumber, password) {
// Prevent people from submitting their password when something isn't right.
if (this.isBusy()) return;
onPasswordLogin: async function(username, phoneCountry, phoneNumber, password) {
if (!this.state.serverIsAlive) {
this.setState({busy: true});
// Do a quick liveliness check on the URLs
try {
await AutoDiscoveryUtils.validateServerConfigWithStaticUrls(
this.props.serverConfig.hsUrl,
this.props.serverConfig.isUrl,
);
this.setState({serverIsAlive: true, errorText: ""});
} catch (e) {
this.setState({
busy: false,
...AutoDiscoveryUtils.authComponentStateForError(e),
});
if (this.state.serverErrorIsFatal) {
Copy link
Contributor

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.

Copy link
Member Author

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...

Copy link
Contributor

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.

Copy link
Member Author

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...

Copy link
Contributor

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.

Copy link
Member Author

@turt2live turt2live Jul 15, 2019 •
edited
Loading

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.

return; // Server is dead - do not continue.
}
}

// Prevent people from submitting their password when something isn't right.
if (!this.state.serverIsAlive) {
return;
}
}

this.setState({
busy: true,
Expand Down