From 577c3168e69712f0ed1ef2b0bf795a70663ef5be Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 23 Aug 2021 15:54:06 +0200 Subject: [PATCH 1/5] make queryLogin abortable --- src/matrix/SessionContainer.js | 10 ++++++--- src/utils/AbortableOperation.ts | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) create mode 100644 src/utils/AbortableOperation.ts diff --git a/src/matrix/SessionContainer.js b/src/matrix/SessionContainer.js index 27776d37..ab20a4eb 100644 --- a/src/matrix/SessionContainer.js +++ b/src/matrix/SessionContainer.js @@ -15,6 +15,7 @@ limitations under the License. */ import {createEnum} from "../utils/enum.js"; +import {AbortableOperation} from "../utils/AbortableOperation"; import {ObservableValue} from "../observable/ObservableValue.js"; import {HomeServerApi} from "./net/HomeServerApi.js"; import {Reconnector, ConnectionStatus} from "./net/Reconnector.js"; @@ -53,6 +54,7 @@ export const LoginFailure = createEnum( "Unknown", ); + export class SessionContainer { constructor({platform, olmPromise, workerPromise}) { this._platform = platform; @@ -121,11 +123,13 @@ export class SessionContainer { return result; } - async queryLogin(homeServer) { + queryLogin(homeServer) { const normalizedHS = normalizeHomeserver(homeServer); const hsApi = new HomeServerApi({homeServer: normalizedHS, request: this._platform.request}); - const response = await hsApi.getLoginFlows().response(); - return this._parseLoginOptions(response, normalizedHS); + return new AbortableOperation(async setAbortable => { + const response = await setAbortable(hsApi.getLoginFlows()).response(); + return this._parseLoginOptions(response, normalizedHS); + }); } async startWithLogin(loginMethod) { diff --git a/src/utils/AbortableOperation.ts b/src/utils/AbortableOperation.ts new file mode 100644 index 00000000..0cc49e10 --- /dev/null +++ b/src/utils/AbortableOperation.ts @@ -0,0 +1,40 @@ +/* +Copyright 2020 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +interface IAbortable { + abort(); +} + +type RunFn = (setAbortable: (a: IAbortable) => typeof a) => T; + +export class AbortableOperation { + public readonly result: T; + private _abortable: IAbortable | null; + + constructor(run: RunFn) { + this._abortable = null; + const setAbortable = abortable => { + this._abortable = abortable; + return abortable; + }; + this.result = run(setAbortable); + } + + abort() { + this._abortable?.abort(); + this._abortable = null; + } +} From 8eab9ab28b33884da9ba0d18a3b18e848056e55c Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 23 Aug 2021 15:54:40 +0200 Subject: [PATCH 2/5] add 2s timeout on input of homeserver to also query the homeserver, in addition to change event --- src/domain/login/LoginViewModel.js | 84 +++++++++++++++++--------- src/platform/web/ui/login/LoginView.js | 7 ++- 2 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/domain/login/LoginViewModel.js b/src/domain/login/LoginViewModel.js index 85dddb79..8827a511 100644 --- a/src/domain/login/LoginViewModel.js +++ b/src/domain/login/LoginViewModel.js @@ -39,8 +39,9 @@ export class LoginViewModel extends ViewModel { this._errorMessage = ""; this._hideHomeserver = false; this._isBusy = false; - this._isFetchingLoginOptions = false; - this._createViewModels(this._homeserver); + this._abortHomeserverQueryTimeout = null; + this._abortQueryOperation = null; + this._initViewModels(); } get passwordLoginViewModel() { return this._passwordLoginViewModel; } @@ -51,13 +52,13 @@ export class LoginViewModel extends ViewModel { get showHomeserver() { return !this._hideHomeserver; } get loadViewModel() {return this._loadViewModel; } get isBusy() { return this._isBusy; } - get isFetchingLoginOptions() { return this._isFetchingLoginOptions; } + get isFetchingLoginOptions() { return !!this._abortQueryOperation; } goBack() { this.navigation.push("session"); } - async _createViewModels(homeserver) { + async _initViewModels() { if (this._loginToken) { this._hideHomeserver = true; this._completeSSOLoginViewModel = this.track(new CompleteSSOLoginViewModel( @@ -70,27 +71,7 @@ export class LoginViewModel extends ViewModel { this.emitChange("completeSSOLoginViewModel"); } else { - this._errorMessage = ""; - try { - this._isFetchingLoginOptions = true; - this.emitChange("isFetchingLoginOptions"); - this._loginOptions = await this._sessionContainer.queryLogin(homeserver); - } - catch (e) { - this._loginOptions = null; - } - this._isFetchingLoginOptions = false; - this.emitChange("isFetchingLoginOptions"); - if (this._loginOptions) { - if (this._loginOptions.sso) { this._showSSOLogin(); } - if (this._loginOptions.password) { this._showPasswordLogin(); } - if (!this._loginOptions.sso && !this._loginOptions.password) { - this._showError("This homeserver neither supports SSO nor Password based login flows"); - } - } - else { - this._showError("Could not query login methods supported by the homeserver"); - } + await this.queryHomeServer(); } } @@ -175,12 +156,59 @@ export class LoginViewModel extends ViewModel { this.emitChange("disposeViewModels"); } - updateHomeServer(newHomeserver) { + async setHomeServerInput(newHomeserver) { + this._homeserver = newHomeserver; + // abort ongoing query, if any + this._abortQueryOperation = this.disposeTracked(this._abortQueryOperation); + this.emitChange("isFetchingLoginOptions"); + this.disposeTracked(this._abortHomeserverQueryTimeout); + const timeout = this.clock.createTimeout(2000); + this._abortHomeserverQueryTimeout = this.track(() => timeout.abort()); + try { + await timeout.elapsed(); + } catch (err) { + if (err.name === "AbortError") { + return; // still typing, don't query + } else { + throw err; + } + } + this._abortHomeserverQueryTimeout = this.disposeTracked(this._abortHomeserverQueryTimeout); + this.queryHomeServer(); + } + + async queryHomeServer() { this._errorMessage = ""; this.emitChange("errorMessage"); - this._homeserver = newHomeserver; + this._abortQueryOperation = this.disposeTracked(this._abortQueryOperation); this._disposeViewModels(); - this._createViewModels(newHomeserver); + try { + const queryOperation = this._sessionContainer.queryLogin(this._homeserver); + this._abortQueryOperation = this.track(() => queryOperation.abort()); + this.emitChange("isFetchingLoginOptions"); + this._loginOptions = await queryOperation.result; + } + catch (e) { + console.log("error", e); + if (e.name === "AbortError") { + return; //aborted, bail out + } else { + this._loginOptions = null; + } + } finally { + this._abortQueryOperation = this.disposeTracked(this._abortQueryOperation); + this.emitChange("isFetchingLoginOptions"); + } + if (this._loginOptions) { + if (this._loginOptions.sso) { this._showSSOLogin(); } + if (this._loginOptions.password) { this._showPasswordLogin(); } + if (!this._loginOptions.sso && !this._loginOptions.password) { + this._showError("This homeserver supports neither SSO nor password based login flows"); + } + } + else { + this._showError("Could not query login methods supported by the homeserver"); + } } dispose() { diff --git a/src/platform/web/ui/login/LoginView.js b/src/platform/web/ui/login/LoginView.js index 3d8e8470..df92cb67 100644 --- a/src/platform/web/ui/login/LoginView.js +++ b/src/platform/web/ui/login/LoginView.js @@ -36,7 +36,6 @@ export class LoginView extends TemplateView { t.mapView(vm => vm.completeSSOLoginViewModel, vm => vm ? new CompleteSSOView(vm) : null), t.if(vm => vm.showHomeserver, (t, vm) => t.div({ className: "LoginView_sso form form-row" }, [ - t.if(vm => vm.errorMessage, (t, vm) => t.p({className: "error"}, vm.i18n(vm.errorMessage))), t.label({for: "homeserver"}, vm.i18n`Homeserver`), t.input({ id: "homeserver", @@ -44,8 +43,10 @@ export class LoginView extends TemplateView { placeholder: vm.i18n`Your matrix homeserver`, value: vm.homeserver, disabled, - onChange: event => vm.updateHomeServer(event.target.value), - }) + onInput: () => vm.setHomeServerInput(event.target.value), + onChange: event => vm.queryHomeServer(), + }), + t.if(vm => vm.errorMessage, (t, vm) => t.p({className: "error"}, vm.i18n(vm.errorMessage))), ] )), t.if(vm => vm.isFetchingLoginOptions, t => t.div({className: "LoginView_query-spinner"}, [spinner(t), t.p("Fetching available login options...")])), From d1301fa642ebb2d7a9b8dcccd20cfd017cf09bcb Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 23 Aug 2021 15:57:16 +0200 Subject: [PATCH 3/5] input is not needed in the name here --- src/domain/login/LoginViewModel.js | 2 +- src/platform/web/ui/login/LoginView.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/domain/login/LoginViewModel.js b/src/domain/login/LoginViewModel.js index 8827a511..27dd47c3 100644 --- a/src/domain/login/LoginViewModel.js +++ b/src/domain/login/LoginViewModel.js @@ -156,7 +156,7 @@ export class LoginViewModel extends ViewModel { this.emitChange("disposeViewModels"); } - async setHomeServerInput(newHomeserver) { + async setHomeServer(newHomeserver) { this._homeserver = newHomeserver; // abort ongoing query, if any this._abortQueryOperation = this.disposeTracked(this._abortQueryOperation); diff --git a/src/platform/web/ui/login/LoginView.js b/src/platform/web/ui/login/LoginView.js index df92cb67..498cb574 100644 --- a/src/platform/web/ui/login/LoginView.js +++ b/src/platform/web/ui/login/LoginView.js @@ -43,7 +43,7 @@ export class LoginView extends TemplateView { placeholder: vm.i18n`Your matrix homeserver`, value: vm.homeserver, disabled, - onInput: () => vm.setHomeServerInput(event.target.value), + onInput: () => vm.setHomeServer(event.target.value), onChange: event => vm.queryHomeServer(), }), t.if(vm => vm.errorMessage, (t, vm) => t.p({className: "error"}, vm.i18n(vm.errorMessage))), From 993bc36096543b5770b5305bc5c2f57a0621c0cb Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 23 Aug 2021 16:01:39 +0200 Subject: [PATCH 4/5] dont query 2nd time after losing focus --- src/domain/login/LoginViewModel.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/domain/login/LoginViewModel.js b/src/domain/login/LoginViewModel.js index 27dd47c3..d065b139 100644 --- a/src/domain/login/LoginViewModel.js +++ b/src/domain/login/LoginViewModel.js @@ -180,6 +180,9 @@ export class LoginViewModel extends ViewModel { async queryHomeServer() { this._errorMessage = ""; this.emitChange("errorMessage"); + // if query is called before the typing timeout hits (e.g. field lost focus), cancel the timeout so we don't query again. + this._abortHomeserverQueryTimeout = this.disposeTracked(this._abortHomeserverQueryTimeout); + // cancel ongoing query operation, if any this._abortQueryOperation = this.disposeTracked(this._abortQueryOperation); this._disposeViewModels(); try { From 9760a4540e7894961e4094e82f9523dd44d4dc9a Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Mon, 23 Aug 2021 16:04:00 +0200 Subject: [PATCH 5/5] remove debug log --- src/domain/login/LoginViewModel.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/domain/login/LoginViewModel.js b/src/domain/login/LoginViewModel.js index d065b139..cd75ca87 100644 --- a/src/domain/login/LoginViewModel.js +++ b/src/domain/login/LoginViewModel.js @@ -192,7 +192,6 @@ export class LoginViewModel extends ViewModel { this._loginOptions = await queryOperation.result; } catch (e) { - console.log("error", e); if (e.name === "AbortError") { return; //aborted, bail out } else {