Fix saferFetch retry handling (replayed from bad merge)

src/Asset.js

@@ -158,4 +158,4 @@ class Asset {
     }
 }
 
-module.exports = Asset;
+module.exports = Asset;

src/FetchTool.js

@@ -1,5 +1,6 @@
 const {scratchFetch} = require('./scratchFetch');
 const saferFetch = require('./safer-fetch');
+const isNullResponse = require('./isNullResponse');
 
 /**
  * @typedef {Request & {withCredentials: boolean}} ScratchSendRequest
@@ -27,7 +28,7 @@ class FetchTool {
         return saferFetch(url, Object.assign({method: 'GET'}, options))
             .then(result => {
                 if (result.ok) return result.arrayBuffer().then(b => new Uint8Array(b));
-                if (result.status === 404) return null;
+                if (isNullResponse(result)) return null;
                 return Promise.reject(result.status); // TODO: we should throw a proper error
             });
     }
@@ -57,4 +58,4 @@ class FetchTool {
     }
 }
 
-module.exports = FetchTool;
+module.exports = FetchTool;

src/FetchWorkerTool.worker.js

@@ -1,5 +1,6 @@
 /* eslint-env worker */
 
+const isNullResponse = require('./isNullResponse');
 const saferFetch = require('./safer-fetch');
 
 const complete = [];
@@ -37,7 +38,7 @@ const onMessage = ({data: job}) => {
     saferFetch(job.url, job.options)
         .then(result => {
             if (result.ok) return result.arrayBuffer();
-            if (result.status === 404) return null;
+            if (isNullResponse(result)) return null;
             return Promise.reject(result.status);
         })
         .then(buffer => complete.push({id: job.id, buffer}))
@@ -47,4 +48,4 @@ const onMessage = ({data: job}) => {
 
 // crossFetch means "fetch" is now always supported
 postMessage({support: {fetch: true}});
-self.addEventListener('message', onMessage);
+self.addEventListener('message', onMessage);

src/isNullResponse.js

@@ -0,0 +1,16 @@
+/**
+ * @param {Response} response the response from fetch()
+ * @returns {boolean} true if the response is a "null response" where we successfully talked to the
+ * source, but the source has no data for us.
+ */
+const isNullResponse = response => (
+    // can't access, eg. due to expired/missing project token
+    response.status === 403 ||
+
+    // assets does not exist
+    // assets.scratch.mit.edu also returns 503 for missing assets
+    response.status === 404 ||
+    response.status === 503
+);
+
+module.exports = isNullResponse;

src/safer-fetch.js

@@ -1,35 +1,31 @@
-/* eslint-env browser */
 /* eslint-disable no-use-before-define */
 
-// This throttles and retries fetch() to mitigate the effect of random network errors and
+const {scratchFetch} = require('./scratchFetch');
+
+// This throttles and retries scratchFetch() to mitigate the effect of random network errors and
 // random browser errors (especially in Chrome)
 
 let currentFetches = 0;
 const queue = [];
 
+const sleep = ms => new Promise(resolve => setTimeout(resolve, ms));
+
 const startNextFetch = ([resolve, url, options]) => {
     let firstError;
     let failedAttempts = 0;
 
-    const attemptToFetch = () => fetch(url, options)
-        .then(result => {
-            // In a macOS WKWebView, requests from file: URLs to other file: URLs always have status: 0 and ok: false
-            // even though the requests were successful. If the requested file doesn't exist, fetch() rejects instead.
-            // We aren't aware of any other cases where fetch() can resolve with status 0, so this should be safe.
-            if (result.ok || result.status === 0) return result.arrayBuffer();
-            if (result.status === 404) return null;
-            return Promise.reject(result.status);
-        })
-        .then(buffer => {
-            currentFetches--;
-            checkStartNextFetch();
-            return buffer;
-        })
+    const done = result => {
+        currentFetches--;
+        checkStartNextFetch();
+        resolve(result);
+    };
+
+    const attemptToFetch = () => scratchFetch(url, options)
+        .then(done)
         .catch(error => {
-            if (error === 403) {
-                // Retrying this request will not help, so return an error now.
-                throw error;
-            }
+            // If fetch() errors, it means there was a network error of some sort.
+            // This is worth retrying, especially as some browser will randomly fail requests
+            // if we send too many at once (as we do).
 
             console.warn(`Attempt to fetch ${url} failed`, error);
             if (!firstError) {
@@ -38,16 +34,14 @@ const startNextFetch = ([resolve, url, options]) => {
 
             if (failedAttempts < 2) {
                 failedAttempts++;
-                return new Promise(cb => setTimeout(cb, (failedAttempts + Math.random() - 1) * 5000))
-                    .then(attemptToFetch);
+                sleep((failedAttempts + Math.random() - 1) * 5000).then(attemptToFetch);
+                return;
             }
 
-            currentFetches--;
-            checkStartNextFetch();
-            throw firstError;
+            done(Promise.reject(firstError));
         });
 
-    return resolve(attemptToFetch());
+    attemptToFetch();
 };
 
 const checkStartNextFetch = () => {
@@ -57,9 +51,9 @@ const checkStartNextFetch = () => {
     }
 };
 
-const saferFetchAsArrayBuffer = (url, options) => new Promise(resolve => {
+const saferFetch = (url, options) => new Promise(resolve => {
     queue.push([resolve, url, options]);
     checkStartNextFetch();
 });
 
-module.exports = saferFetchAsArrayBuffer;
+module.exports = saferFetch;