Merge pull request #323 from vector-im/bwindels/service-worker-aborts

Map service worker aborts as network errors, so sync does not halt

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, encodeBody} from "./common.js";
-import {encodeQueryParams} from "./common.js";
+import {HomeServerRequest} from "./HomeServerRequest.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);
-    }
-}
 
 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() {
+import {Request as MockRequest} from "../../mocks/Request.js";
-    function createRequestMock(result) {
-        return function() {
-            return {
-                abort() {},
-                response() {
-                    return Promise.resolve(result);
-                }
-            }
-        }
-    }
 
+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();

src/matrix/net/HomeServerRequest.js

@@ -0,0 +1,140 @@
+/*
+Copyright 2020 Bruno Windels <bruno@windels.cloud>
+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);
+        },
+    };
+}

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

src/mocks/Request.js

@@ -0,0 +1,41 @@
+/*
+Copyright 2020 Bruno Windels <bruno@windels.cloud>
+
+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;
+    }
+}

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

src/platform/web/dom/request/timeout.js

@@ -1,51 +0,0 @@
-/*
-Copyright 2020 Bruno Windels <bruno@windels.cloud>
-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;
-            }
-        }
-    );
-}

src/utils/timeout.js

@@ -0,0 +1,87 @@
+/*
+Copyright 2020 Bruno Windels <bruno@windels.cloud>
+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);
+        }
+    }
+
+}