Fix saferFetch retry behavior

Author: GarboMuffin

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

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

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

@@ -8,21 +8,24 @@ const {scratchFetch} = require('./scratchFetch');
 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 done = result => {
+        currentFetches--;
+        checkStartNextFetch();
+        resolve(result);
+    };
+
     const attemptToFetch = () => scratchFetch(url, options)
-        .then(result => {
-            currentFetches--;
-            checkStartNextFetch();
-            return result;
-        })
+        .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) {
@@ -31,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 = () => {