diff --git a/src/matrix/net/HomeServerApi.js b/src/matrix/net/HomeServerApi.js index f8dfa517..cbcb55ab 100644 --- a/src/matrix/net/HomeServerApi.js +++ b/src/matrix/net/HomeServerApi.js @@ -15,84 +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) { - const err = new Error(`Unexpectedly aborted, see #187.`); - 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}) { @@ -143,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, @@ -157,7 +81,7 @@ export class HomeServerApi { }); } - return wrapper; + return hsRequest; } _unauthedRequest(method, url, queryParams, body, options) { @@ -264,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..97728c28 --- /dev/null +++ b/src/matrix/net/HomeServerRequest.js @@ -0,0 +1,140 @@ +/* +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 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), 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 + // 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 + // 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; + } +} 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..6bfc0d7e --- /dev/null +++ b/src/utils/timeout.js @@ -0,0 +1,87 @@ +/* +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 as MockClock} from "../mocks/Clock.js"; +import {Request as MockRequest} from "../mocks/Request.js"; +import {AbortError} from "../matrix/error.js"; +export function tests() { + return { + "ConnectionError on timeout": async assert => { + 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 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 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); + } + } + +}