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; + } +}