From 23417480230a97ad6e7f5f8dd9feda88d5e3db32 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 9 Apr 2021 14:09:48 +0200 Subject: [PATCH 1/5] add some tests to timeout code --- src/platform/web/dom/request/fetch.js | 4 +- src/platform/web/dom/request/timeout.js | 51 ------------ src/utils/timeout.js | 100 ++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 52 deletions(-) delete mode 100644 src/platform/web/dom/request/timeout.js create mode 100644 src/utils/timeout.js diff --git a/src/platform/web/dom/request/fetch.js b/src/platform/web/dom/request/fetch.js index 1222c41b..66f1a148 100644 --- a/src/platform/web/dom/request/fetch.js +++ b/src/platform/web/dom/request/fetch.js @@ -19,7 +19,7 @@ import { AbortError, ConnectionError } from "../../../../matrix/error.js"; -import {abortOnTimeout} from "./timeout.js"; +import {abortOnTimeout} from "../../../../utils/timeout.js"; import {addCacheBuster} from "./common.js"; import {xhrRequest} from "./xhr.js"; @@ -121,6 +121,8 @@ export function createFetchRequest(createTimeout, serviceWorkerHandler) { return {status, body}; }, err => { if (err.name === "AbortError") { + // map DOMException with name AbortError to our own AbortError type + // as we don't want DOMExceptions in the protocol layer. throw new AbortError(); } else if (err instanceof TypeError) { // Network errors are reported as TypeErrors, see diff --git a/src/platform/web/dom/request/timeout.js b/src/platform/web/dom/request/timeout.js deleted file mode 100644 index 030e4150..00000000 --- a/src/platform/web/dom/request/timeout.js +++ /dev/null @@ -1,51 +0,0 @@ -/* -Copyright 2020 Bruno Windels -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. -*/ - -import { - AbortError, - ConnectionError -} from "../../../../matrix/error.js"; - - -export function abortOnTimeout(createTimeout, timeoutAmount, requestResult, responsePromise) { - const timeout = createTimeout(timeoutAmount); - // abort request if timeout finishes first - let timedOut = false; - timeout.elapsed().then( - () => { - timedOut = true; - requestResult.abort(); - }, - () => {} // ignore AbortError when timeout is aborted - ); - // abort timeout if request finishes first - return responsePromise.then( - response => { - timeout.abort(); - return response; - }, - err => { - timeout.abort(); - // map error to TimeoutError - if (err.name === "AbortError" && timedOut) { - throw new ConnectionError(`Request timed out after ${timeoutAmount}ms`, true); - } else { - throw err; - } - } - ); -} diff --git a/src/utils/timeout.js b/src/utils/timeout.js new file mode 100644 index 00000000..886d6a50 --- /dev/null +++ b/src/utils/timeout.js @@ -0,0 +1,100 @@ +/* +Copyright 2020 Bruno Windels +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. +*/ + +import {ConnectionError} from "../matrix/error.js"; + + +export function abortOnTimeout(createTimeout, timeoutAmount, requestResult, responsePromise) { + const timeout = createTimeout(timeoutAmount); + // abort request if timeout finishes first + let timedOut = false; + timeout.elapsed().then( + () => { + timedOut = true; + requestResult.abort(); + }, + () => {} // ignore AbortError when timeout is aborted + ); + // abort timeout if request finishes first + return responsePromise.then( + response => { + timeout.abort(); + return response; + }, + err => { + timeout.abort(); + // map error to TimeoutError + if (err.name === "AbortError" && timedOut) { + throw new ConnectionError(`Request timed out after ${timeoutAmount}ms`, true); + } else { + throw err; + } + } + ); +} + +// because impunity only takes one entrypoint currently, +// these tests aren't run by yarn test as that does not +// include platform specific code, +// and this file is only included by platform specific code, +// see how to run in package.json and replace src/main.js with this file. +import {Clock} from "../mocks/Clock.js"; +import {AbortError} from "../matrix/error.js"; +export function tests() { + function createRequest() { + let request = { + abort() { + this.aborted = true; + this.reject(new AbortError()); + } + }; + request.responsePromise = new Promise((resolve, reject) => { + request.resolve = resolve; + request.reject = reject; + }); + return request; + } + + return { + "ConnectionError on timeout": async assert => { + const clock = new Clock(); + const request = createRequest(); + const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.responsePromise); + clock.elapse(10000); + await assert.rejects(promise, ConnectionError); + assert(request.aborted); + }, + "Abort is canceled once response resolves": async assert => { + const clock = new Clock(); + const request = createRequest(); + const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.responsePromise); + request.resolve(5); + clock.elapse(10000); + assert(!request.aborted); + assert.equal(await promise, 5); + }, + "AbortError from request is not mapped to ConnectionError": async assert => { + const clock = new Clock(); + const request = createRequest(); + const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.responsePromise); + request.reject(new AbortError()); + assert(!request.aborted); + assert.rejects(promise, AbortError); + } + } + +} From 2b1f4866a98c125c5c159a89b5fa98405aee2e2b Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 9 Apr 2021 14:25:19 +0200 Subject: [PATCH 2/5] map unexpected fetch AbortError to ConnectionError, so doesn't stop sync --- src/matrix/net/HomeServerApi.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/matrix/net/HomeServerApi.js b/src/matrix/net/HomeServerApi.js index f8dfa517..b93dac91 100644 --- a/src/matrix/net/HomeServerApi.js +++ b/src/matrix/net/HomeServerApi.js @@ -47,7 +47,23 @@ class RequestWrapper { }, err => { // if this._requestResult is still set, the abort error came not from calling abort here if (err.name === "AbortError" && this._requestResult) { - const err = new Error(`Unexpectedly aborted, see #187.`); + // The service worker sometimes (only on Firefox, on long, large request, + // perhaps it has its own timeout?) aborts the request, see #187. + // When it happens, the best thing to do seems to be to retry. + // + // In the service worker, we will also actively abort requests when trying to + // get a new service worker to activate, as the service worker will only be replaced + // when there are no more (fetch) events for the current one to handle. + // + // In that case, the request function (in fetch.js) will check + // the haltRequests flag on the service worker handler, and it will + // actually not do any requests, as that would break the update process. + // + // So it is OK to return a timeout ConnectionError here. + // If we're updating the service worker, the /versions polling will + // actually be blocked at the fetch level because haltRequests is set. + // And for #187, retrying is the right thing to do. + const err = new ConnectionError(`Service worker aborted, either updating or hit #187.`, true); log?.catch(err); throw err; } else { From c36e812360e12a6bdc1be953a275d3d5efa0753f Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 9 Apr 2021 15:15:28 +0200 Subject: [PATCH 3/5] move RequestWrapper to own file and add tests, improve comments, and and don't use timeout connection error as that's not what happens if aborted request from service worker is reported as TypeError either. --- src/matrix/net/HomeServerApi.js | 117 ++--------------------- src/matrix/net/HomeServerRequest.js | 139 ++++++++++++++++++++++++++++ src/matrix/net/common.js | 20 ++++ src/mocks/Request.js | 41 ++++++++ 4 files changed, 208 insertions(+), 109 deletions(-) create mode 100644 src/matrix/net/HomeServerRequest.js create mode 100644 src/mocks/Request.js diff --git a/src/matrix/net/HomeServerApi.js b/src/matrix/net/HomeServerApi.js index b93dac91..cbcb55ab 100644 --- a/src/matrix/net/HomeServerApi.js +++ b/src/matrix/net/HomeServerApi.js @@ -15,100 +15,8 @@ See the License for the specific language governing permissions and limitations under the License. */ -import {HomeServerError, ConnectionError} from "../error.js"; -import {encodeQueryParams} from "./common.js"; - -class RequestWrapper { - constructor(method, url, requestResult, log) { - this._log = log; - this._requestResult = requestResult; - this._promise = requestResult.response().then(response => { - log?.set("status", response.status); - // ok? - if (response.status >= 200 && response.status < 300) { - log?.finish(); - return response.body; - } else { - if (response.status >= 500) { - const err = new ConnectionError(`Internal Server Error`); - log?.catch(err); - throw err; - } else if (response.status >= 400 && !response.body?.errcode) { - const err = new ConnectionError(`HTTP error status ${response.status} without errcode in body, assume this is a load balancer complaining the server is offline.`); - log?.catch(err); - throw err; - } else { - const err = new HomeServerError(method, url, response.body, response.status); - log?.set("errcode", err.errcode); - log?.catch(err); - throw err; - } - } - }, err => { - // if this._requestResult is still set, the abort error came not from calling abort here - if (err.name === "AbortError" && this._requestResult) { - // The service worker sometimes (only on Firefox, on long, large request, - // perhaps it has its own timeout?) aborts the request, see #187. - // When it happens, the best thing to do seems to be to retry. - // - // In the service worker, we will also actively abort requests when trying to - // get a new service worker to activate, as the service worker will only be replaced - // when there are no more (fetch) events for the current one to handle. - // - // In that case, the request function (in fetch.js) will check - // the haltRequests flag on the service worker handler, and it will - // actually not do any requests, as that would break the update process. - // - // So it is OK to return a timeout ConnectionError here. - // If we're updating the service worker, the /versions polling will - // actually be blocked at the fetch level because haltRequests is set. - // And for #187, retrying is the right thing to do. - const err = new ConnectionError(`Service worker aborted, either updating or hit #187.`, true); - log?.catch(err); - throw err; - } else { - if (err.name === "ConnectionError") { - log?.set("timeout", err.isTimeout); - } - log?.catch(err); - throw err; - } - }); - } - - abort() { - if (this._requestResult) { - this._log?.set("aborted", true); - this._requestResult.abort(); - // to mark that it was on purpose in above rejection handler - this._requestResult = null; - } - } - - response() { - return this._promise; - } -} - -function encodeBody(body) { - if (body.nativeBlob && body.mimeType) { - const blob = body; - return { - mimeType: blob.mimeType, - body: blob, // will be unwrapped in request fn - length: blob.size - }; - } else if (typeof body === "object") { - const json = JSON.stringify(body); - return { - mimeType: "application/json", - body: json, - length: body.length - }; - } else { - throw new Error("Unknown body type: " + body); - } -} +import {encodeQueryParams, encodeBody} from "./common.js"; +import {HomeServerRequest} from "./HomeServerRequest.js"; export class HomeServerApi { constructor({homeServer, accessToken, request, createTimeout, reconnector}) { @@ -159,10 +67,10 @@ export class HomeServerApi { format: "json" // response format }); - const wrapper = new RequestWrapper(method, url, requestResult, log); + const hsRequest = new HomeServerRequest(method, url, requestResult, log); if (this._reconnector) { - wrapper.response().catch(err => { + hsRequest.response().catch(err => { // Some endpoints such as /sync legitimately time-out // (which is also reported as a ConnectionError) and will re-attempt, // but spinning up the reconnector in this case is ok, @@ -173,7 +81,7 @@ export class HomeServerApi { }); } - return wrapper; + return hsRequest; } _unauthedRequest(method, url, queryParams, body, options) { @@ -280,22 +188,13 @@ export class HomeServerApi { } } -export function tests() { - function createRequestMock(result) { - return function() { - return { - abort() {}, - response() { - return Promise.resolve(result); - } - } - } - } +import {Request as MockRequest} from "../../mocks/Request.js"; +export function tests() { return { "superficial happy path for GET": async assert => { const hsApi = new HomeServerApi({ - request: createRequestMock({body: 42, status: 200}), + request: () => new MockRequest().respond(200, 42), homeServer: "https://hs.tld" }); const result = await hsApi._get("foo", null, null, null).response(); diff --git a/src/matrix/net/HomeServerRequest.js b/src/matrix/net/HomeServerRequest.js new file mode 100644 index 00000000..c059c5a6 --- /dev/null +++ b/src/matrix/net/HomeServerRequest.js @@ -0,0 +1,139 @@ +/* +Copyright 2020 Bruno Windels +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. +*/ + +import {HomeServerError, ConnectionError} from "../error.js"; + +export class HomeServerRequest { + constructor(method, url, sourceRequest, log) { + this._log = log; + this._sourceRequest = sourceRequest; + this._promise = sourceRequest.response().then(response => { + log?.set("status", response.status); + // ok? + if (response.status >= 200 && response.status < 300) { + log?.finish(); + return response.body; + } else { + if (response.status >= 500) { + const err = new ConnectionError(`Internal Server Error`); + log?.catch(err); + throw err; + } else if (response.status >= 400 && !response.body?.errcode) { + const err = new ConnectionError(`HTTP error status ${response.status} without errcode in body, assume this is a load balancer complaining the server is offline.`); + log?.catch(err); + throw err; + } else { + const err = new HomeServerError(method, url, response.body, response.status); + log?.set("errcode", err.errcode); + log?.catch(err); + throw err; + } + } + }, err => { + // if this._sourceRequest is still set, the abort error came not from calling abort here + if (err.name === "AbortError" && this._sourceRequest) { + // The service worker sometimes (only on Firefox, on long, large request, + // perhaps it has its own timeout?) aborts the request, see #187. + // When it happens, the best thing to do seems to be to retry. + // + // In the service worker, we will also actively abort requests + // (this may surface in the app as a TypeError, which already gets mapped + // to a ConnectionError in the request function, or an AbortError, + // depending on the browser) + // when trying to get a new service worker to activate, + // as the service worker will only be replaced when there are + // no more (fetch) events for the current one to handle. + // + // In that case, the request function (in fetch.js) will check + // the haltRequests flag on the service worker handler, and it will + // actually not do any requests, as that would break the update process. + // + // So it is OK to return a ConnectionError here. + // If we're updating the service worker, the /versions polling will + // actually be blocked at the fetch level because haltRequests is set. + // And for #187, retrying is the right thing to do. + const err = new ConnectionError(`Service worker aborted, either updating or hit #187.`); + log?.catch(err); + throw err; + } else { + if (err.name === "ConnectionError") { + log?.set("timeout", err.isTimeout); + } + log?.catch(err); + throw err; + } + }); + } + + abort() { + if (this._sourceRequest) { + this._log?.set("aborted", true); + this._sourceRequest.abort(); + // to mark that it was on purpose in above rejection handler + this._sourceRequest = null; + } + } + + response() { + return this._promise; + } +} + +import {Request as MockRequest} from "../../mocks/Request.js"; +import {AbortError} from "../error.js"; + +export function tests() { + return { + "Response is passed through": async assert => { + const request = new MockRequest(); + const hsRequest = new HomeServerRequest("GET", "https://hs.tld/foo", request); + request.respond(200, "foo"); + assert.equal(await hsRequest.response(), "foo"); + }, + "Unexpected AbortError is mapped to ConnectionError": async assert => { + const request = new MockRequest(); + const hsRequest = new HomeServerRequest("GET", "https://hs.tld/foo", request); + request.reject(new AbortError()); + await assert.rejects(hsRequest.response(), ConnectionError); + }, + "500 response is mapped to ConnectionError": async assert => { + const request = new MockRequest(); + const hsRequest = new HomeServerRequest("GET", "https://hs.tld/foo", request); + request.respond(500); + await assert.rejects(hsRequest.response(), ConnectionError); + }, + "4xx response is mapped to HomeServerError": async assert => { + const request = new MockRequest(); + const hsRequest = new HomeServerRequest("GET", "https://hs.tld/foo", request); + request.respond(400, {errcode: "FOO"}); + await assert.rejects(hsRequest.response(), HomeServerError); + }, + "4xx response without errcode is mapped to ConnectionError": async assert => { + const request = new MockRequest(); + const hsRequest = new HomeServerRequest("GET", "https://hs.tld/foo", request); + request.respond(400); + await assert.rejects(hsRequest.response(), ConnectionError); + }, + "Other errors are passed through": async assert => { + class MyError extends Error {} + const request = new MockRequest(); + const hsRequest = new HomeServerRequest("GET", "https://hs.tld/foo", request); + request.reject(new MyError()); + await assert.rejects(hsRequest.response(), MyError); + }, + }; +} diff --git a/src/matrix/net/common.js b/src/matrix/net/common.js index c7a06351..889042ae 100644 --- a/src/matrix/net/common.js +++ b/src/matrix/net/common.js @@ -26,3 +26,23 @@ export function encodeQueryParams(queryParams) { }) .join("&"); } + +export function encodeBody(body) { + if (body.nativeBlob && body.mimeType) { + const blob = body; + return { + mimeType: blob.mimeType, + body: blob, // will be unwrapped in request fn + length: blob.size + }; + } else if (typeof body === "object") { + const json = JSON.stringify(body); + return { + mimeType: "application/json", + body: json, + length: body.length + }; + } else { + throw new Error("Unknown body type: " + body); + } +} diff --git a/src/mocks/Request.js b/src/mocks/Request.js new file mode 100644 index 00000000..da8693b1 --- /dev/null +++ b/src/mocks/Request.js @@ -0,0 +1,41 @@ +/* +Copyright 2020 Bruno Windels + +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. +*/ + +import {AbortError} from "../utils/error.js"; + +export class Request { + constructor() { + this._responsePromise = new Promise((resolve, reject) => { + this.resolve = resolve; + this.reject = reject; + }); + this.aborted = false; + } + + respond(status, body) { + this.resolve({status, body}); + return this; + } + + abort() { + this.aborted = true; + this.reject(new AbortError()); + } + + response() { + return this._responsePromise; + } +} From c604c31032decbf0a048eef88db7f530b6aebc80 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 9 Apr 2021 15:16:43 +0200 Subject: [PATCH 4/5] use external mock for request in timeout tests --- src/utils/timeout.js | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/src/utils/timeout.js b/src/utils/timeout.js index 886d6a50..6bfc0d7e 100644 --- a/src/utils/timeout.js +++ b/src/utils/timeout.js @@ -52,45 +52,32 @@ export function abortOnTimeout(createTimeout, timeoutAmount, requestResult, resp // include platform specific code, // and this file is only included by platform specific code, // see how to run in package.json and replace src/main.js with this file. -import {Clock} from "../mocks/Clock.js"; +import {Clock as MockClock} from "../mocks/Clock.js"; +import {Request as MockRequest} from "../mocks/Request.js"; import {AbortError} from "../matrix/error.js"; export function tests() { - function createRequest() { - let request = { - abort() { - this.aborted = true; - this.reject(new AbortError()); - } - }; - request.responsePromise = new Promise((resolve, reject) => { - request.resolve = resolve; - request.reject = reject; - }); - return request; - } - return { "ConnectionError on timeout": async assert => { - const clock = new Clock(); - const request = createRequest(); - const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.responsePromise); + const clock = new MockClock(); + const request = new MockRequest(); + const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.response()); clock.elapse(10000); await assert.rejects(promise, ConnectionError); assert(request.aborted); }, "Abort is canceled once response resolves": async assert => { - const clock = new Clock(); - const request = createRequest(); - const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.responsePromise); + const clock = new MockClock(); + const request = new MockRequest(); + const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.response()); request.resolve(5); clock.elapse(10000); assert(!request.aborted); assert.equal(await promise, 5); }, "AbortError from request is not mapped to ConnectionError": async assert => { - const clock = new Clock(); - const request = createRequest(); - const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.responsePromise); + const clock = new MockClock(); + const request = new MockRequest(); + const promise = abortOnTimeout(clock.createTimeout, 10000, request, request.response()); request.reject(new AbortError()); assert(!request.aborted); assert.rejects(promise, AbortError); From 606e30fed27723980eb4efb879ace97fb2a56a25 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Fri, 9 Apr 2021 15:24:10 +0200 Subject: [PATCH 5/5] make comment easier to read --- src/matrix/net/HomeServerRequest.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/matrix/net/HomeServerRequest.js b/src/matrix/net/HomeServerRequest.js index c059c5a6..97728c28 100644 --- a/src/matrix/net/HomeServerRequest.js +++ b/src/matrix/net/HomeServerRequest.js @@ -44,27 +44,28 @@ export class HomeServerRequest { } } }, err => { - // if this._sourceRequest is still set, the abort error came not from calling abort here + // if this._sourceRequest is still set, + // the abort error came not from calling abort here if (err.name === "AbortError" && this._sourceRequest) { // The service worker sometimes (only on Firefox, on long, large request, // perhaps it has its own timeout?) aborts the request, see #187. // When it happens, the best thing to do seems to be to retry. // - // In the service worker, we will also actively abort requests + // In the service worker, we will also actively abort all + // ongoing requests when trying to get a new service worker to activate // (this may surface in the app as a TypeError, which already gets mapped // to a ConnectionError in the request function, or an AbortError, - // depending on the browser) - // when trying to get a new service worker to activate, - // as the service worker will only be replaced when there are - // no more (fetch) events for the current one to handle. + // depending on the browser), as the service worker will only be + // replaced when there are no more (fetch) events for the + // current one to handle. // // In that case, the request function (in fetch.js) will check - // the haltRequests flag on the service worker handler, and it will - // actually not do any requests, as that would break the update process. + // the haltRequests flag on the service worker handler, and + // block any new requests, as that would break the update process. // // So it is OK to return a ConnectionError here. // If we're updating the service worker, the /versions polling will - // actually be blocked at the fetch level because haltRequests is set. + // be blocked at the fetch level because haltRequests is set. // And for #187, retrying is the right thing to do. const err = new ConnectionError(`Service worker aborted, either updating or hit #187.`); log?.catch(err);