Merge pull request #281 from vector-im/bwindels/fix-updates

Fix service worker updates stalling

src/platform/web/Platform.js

@@ -107,7 +107,7 @@ export class Platform {
         this.sessionInfoStorage = new SessionInfoStorage("hydrogen_sessions_v1");
         this.estimateStorageUsage = estimateStorageUsage;
         if (typeof fetch === "function") {
-            this.request = createFetchRequest(this.clock.createTimeout);
+            this.request = createFetchRequest(this.clock.createTimeout, this._serviceWorkerHandler);
         } else {
             this.request = xhrRequest;
         }

src/platform/web/dom/ServiceWorkerHandler.js

@@ -26,6 +26,7 @@ export class ServiceWorkerHandler {
         this._registration = null;
         this._registrationPromise = null;
         this._currentController = null;
+        this.haltRequests = false;
     }
 
     setNavigation(navigation) {
@@ -39,10 +40,13 @@ export class ServiceWorkerHandler {
             this._registration = await navigator.serviceWorker.register(path);
             await navigator.serviceWorker.ready;
             this._currentController = navigator.serviceWorker.controller;
-            this._registrationPromise = null;
-            console.log("Service Worker registered");
             this._registration.addEventListener("updatefound", this);
-            this._tryActivateUpdate();
+            this._registrationPromise = null;
+            // do we have a new service worker waiting to activate?
+            if (this._registration.waiting && this._registration.active) {
+                this._proposeUpdate();
+            }
+            console.log("Service Worker registered");
         })();
     }
 
@@ -61,6 +65,10 @@ export class ServiceWorkerHandler {
             this._closeSessionIfNeeded(sessionId).finally(() => {
                 event.source.postMessage({replyTo: data.id});
             });
+        } else if (data.type === "haltRequests") {
+            // this flag is read in fetch.js
+            this.haltRequests = true;
+            event.source.postMessage({replyTo: data.id});
         }
     }
 
@@ -82,15 +90,19 @@ export class ServiceWorkerHandler {
         }
     }
 
-    async _tryActivateUpdate() {
+    async _proposeUpdate() {
-        // we don't do confirm when the tab is hidden because it will block the event loop and prevent
+        if (document.hidden) {
-        // events from the service worker to be processed (like controllerchange when the visible tab applies the update). 
+            return;
-        if (!document.hidden && this._registration.waiting && this._registration.active) {
-            this._registration.waiting.removeEventListener("statechange", this);
-            const version = await this._sendAndWaitForReply("version", null, this._registration.waiting);
-            if (confirm(`Version ${version.version} (${version.buildHash}) is ready to install. Apply now?`)) {
-                this._registration.waiting.postMessage({type: "skipWaiting"}); // will trigger controllerchange event
         }
+        const version = await this._sendAndWaitForReply("version");
+        if (confirm(`Version ${version.version} (${version.buildHash}) is available. Reload to apply?`)) {
+            // prevent any fetch requests from going to the service worker
+            // from any client, so that it is not kept active
+            // when calling skipWaiting on the new one
+            await this._sendAndWaitForReply("haltRequests");
+            // only once all requests are blocked, ask the new
+            // service worker to skipWaiting
+            this._send("skipWaiting", null, this._registration.waiting);
         }
     }
 
@@ -101,11 +113,14 @@ export class ServiceWorkerHandler {
                 break;
             case "updatefound":
                 this._registration.installing.addEventListener("statechange", this);
-                this._tryActivateUpdate();
                 break;
-            case "statechange":
+            case "statechange": {
-                this._tryActivateUpdate();
+                if (event.target.state === "installed") {
+                    this._proposeUpdate();
+                    event.target.removeEventListener("statechange", this);
+                }
                 break;
+            }
             case "controllerchange":
                 if (!this._currentController) {
                     // Clients.claim() in the SW can trigger a controllerchange event
@@ -115,7 +130,7 @@ export class ServiceWorkerHandler {
                 } else {
                     // active service worker changed,
                     // refresh, so we can get all assets 
-                    // (and not some if we would not refresh)
+                    // (and not only some if we would not refresh)
                     // up to date from it
                     document.location.reload();
                 }

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

@@ -51,8 +51,15 @@ class RequestResult {
     }
 }
 
-export function createFetchRequest(createTimeout) {
+export function createFetchRequest(createTimeout, serviceWorkerHandler) {
     return function fetchRequest(url, requestOptions) {
+        if (serviceWorkerHandler?.haltRequests) {
+            // prevent any requests while waiting
+            // for the new service worker to get activated.
+            // Once this happens, the page will be reloaded
+            // by the serviceWorkerHandler so this is fine.
+            return new RequestResult(new Promise(() => {}), {});
+        }
         // fetch doesn't do upload progress yet, delegate to xhr
         if (requestOptions?.uploadProgress) {
             return xhrRequest(url, requestOptions);

src/platform/web/service-worker.template.js

@@ -37,6 +37,13 @@ self.addEventListener('install', function(e) {
     })());
 });
 
+self.addEventListener('activate', (event) => {
+    // on a first page load/sw install,
+    // start using the service worker on all pages straight away
+    self.clients.claim();
+    event.waitUntil(purgeOldCaches());
+});
+
 async function purgeOldCaches() {
     // remove any caches we don't know about
     const keyList = await caches.keys();
@@ -60,15 +67,6 @@ async function purgeOldCaches() {
     }
 }
 
-self.addEventListener('activate', (event) => {
-    event.waitUntil(Promise.all([
-        purgeOldCaches(),
-        // on a first page load/sw install,
-        // start using the service worker on all pages straight away
-        self.clients.claim()
-    ]));
-});
-
 self.addEventListener('fetch', (event) => {
     event.respondWith(handleRequest(event.request));
 });
@@ -85,9 +83,11 @@ function isCacheableThumbnail(url) {
 }
 
 const baseURL = new URL(self.registration.scope);
+let pendingFetchAbortController = new AbortController();
 async function handleRequest(request) {
     try {
         const url = new URL(request.url);
+        // rewrite / to /index.html so it hits the cache
         if (url.origin === baseURL.origin && url.pathname === baseURL.pathname) {
             request = new Request(new URL("index.html", baseURL.href));
         }
@@ -96,15 +96,15 @@ async function handleRequest(request) {
             // use cors so the resource in the cache isn't opaque and uses up to 7mb
             // https://developers.google.com/web/tools/chrome-devtools/progressive-web-apps?utm_source=devtools#opaque-responses
             if (isCacheableThumbnail(url)) {
-                response = await fetch(request, {mode: "cors", credentials: "omit"});
+                response = await fetch(request, {signal: pendingFetchAbortController.signal, mode: "cors", credentials: "omit"});
             } else {
-                response = await fetch(request);
+                response = await fetch(request, {signal: pendingFetchAbortController.signal});
             }
             await updateCache(request, response);
         }
         return response;
     } catch (err) {
-        if (!(err instanceof TypeError)) {
+        if (err.name !== "TypeError" && err.name !== "AbortError") {
             console.error("error in service worker", err);
         }
         throw err;
@@ -172,10 +172,13 @@ self.addEventListener('message', (event) => {
             case "skipWaiting":
                 self.skipWaiting();
                 break;
+            case "haltRequests":
+                event.waitUntil(haltRequests().finally(() => reply()));
+                break;
             case "closeSession":
                 event.waitUntil(
                     closeSession(event.data.payload.sessionId, event.source.id)
-                        .then(() => reply())
+                        .finally(() => reply())
                 );
                 break;
         }
@@ -192,6 +195,16 @@ async function closeSession(sessionId, requestingClientId) {
     }));
 }
 
+async function haltRequests() {
+    // first ask all clients to block sending any more requests
+    const clients = await self.clients.matchAll({type: "window"});
+    await Promise.all(clients.map(client => {
+        return sendAndWaitForReply(client, "haltRequests");
+    }));
+    // and only then abort the current requests
+    pendingFetchAbortController.abort();
+}
+
 const pendingReplies = new Map();
 let messageIdCounter = 0;
 function sendAndWaitForReply(client, type, payload) {