Bug 1534847 - Part 1: Refactor loadSourceText to separate caching from logic. r=jlast
☠☠ backed out by fa6af8c14ee4 ☠ ☠
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 14 Mar 2019 20:38:20 +0000
changeset 521976 7cf6462189e8d4834052afa8fb9201744c5df56a
parent 521975 6e05f81a2f8ff603048062f0ac1943d5aa37a4f4
child 521977 27a3400989c9bce15e4c9b1422637067e34e7c6c
push id10870
push usernbeleuzu@mozilla.com
push dateFri, 15 Mar 2019 20:00:07 +0000
treeherdermozilla-beta@c594aee5b7a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlast
bugs1534847
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1534847 - Part 1: Refactor loadSourceText to separate caching from logic. r=jlast Splitting up this logic makes us less likely to introduce code that would break the caching behavior. If you look closely at these changes, you'll notice that there actually one one early return in this code that would cause us to exit without clearing the 'requests' cache meaning we could get stuck in an infinite loading state. Differential Revision: https://phabricator.services.mozilla.com/D23451
devtools/client/debugger/new/src/actions/sources/loadSourceText.js
--- a/devtools/client/debugger/new/src/actions/sources/loadSourceText.js
+++ b/devtools/client/debugger/new/src/actions/sources/loadSourceText.js
@@ -7,17 +7,16 @@
 import { PROMISE } from "../utils/middleware/promise";
 import { getSource } from "../../selectors";
 import { setBreakpointPositions } from "../breakpoints";
 
 import * as parser from "../../workers/parser";
 import { isLoaded, isOriginal } from "../../utils/source";
 import { Telemetry } from "devtools-modules";
 
-import defer from "../../utils/defer";
 import type { ThunkArgs } from "../types";
 
 import type { Source } from "../../types";
 
 const requests = new Map();
 
 // Measures the time it takes for a source to load
 const loadSourceHistogram = "DEVTOOLS_DEBUGGER_LOAD_SOURCE_MS";
@@ -27,71 +26,78 @@ async function loadSource(state, source:
   if (isOriginal(source)) {
     return sourceMaps.getOriginalSourceText(source);
   }
 
   if (!source.actors.length) {
     throw new Error("No source actor for loadSource");
   }
 
+  telemetry.start(loadSourceHistogram, source);
   const response = await client.sourceContents(source.actors[0]);
   telemetry.finish(loadSourceHistogram, source);
 
   return {
     id: source.id,
     text: response.source,
     contentType: response.contentType || "text/javascript"
   };
 }
 
+async function loadSourceTextPromise(
+  source: Source,
+  { dispatch, getState, client, sourceMaps }: ThunkArgs
+): Promise<?Source> {
+  if (isLoaded(source)) {
+    return source;
+  }
+
+  await dispatch({
+    type: "LOAD_SOURCE_TEXT",
+    sourceId: source.id,
+    [PROMISE]: loadSource(getState(), source, { sourceMaps, client })
+  });
+
+  const newSource = getSource(getState(), source.id);
+  if (!newSource) {
+    return;
+  }
+
+  if (!newSource.isWasm && isLoaded(newSource)) {
+    parser.setSource(newSource);
+    await dispatch(setBreakpointPositions(newSource.id));
+  }
+
+  return newSource;
+}
+
 /**
  * @memberof actions/sources
  * @static
  */
-export function loadSourceText(source: ?Source) {
-  return async ({ dispatch, getState, client, sourceMaps }: ThunkArgs) => {
-    if (!source) {
+export function loadSourceText(inputSource: ?Source) {
+  return async (thunkArgs: ThunkArgs) => {
+    if (!inputSource) {
       return;
     }
+    // This ensures that the falsy check above is preserved into the IIFE
+    // below in a way that Flow is happy with.
+    const source = inputSource;
 
     const id = source.id;
-    // Fetch the source text only once.
-    if (requests.has(id)) {
-      return requests.get(id);
-    }
-
-    if (isLoaded(source)) {
-      return Promise.resolve();
+    let promise = requests.get(id);
+    if (!promise) {
+      promise = (async () => {
+        try {
+          return await loadSourceTextPromise(source, thunkArgs);
+        } catch (e) {
+          // TODO: This swallows errors for now. Ideally we would get rid of
+          // this once we have a better handle on our async state management.
+        } finally {
+          requests.delete(id);
+        }
+      })();
+      requests.set(id, promise);
     }
 
-    const deferred = defer();
-    requests.set(id, deferred.promise);
-
-    telemetry.start(loadSourceHistogram, source);
-    try {
-      await dispatch({
-        type: "LOAD_SOURCE_TEXT",
-        sourceId: source.id,
-        [PROMISE]: loadSource(getState(), source, { sourceMaps, client })
-      });
-    } catch (e) {
-      deferred.resolve();
-      requests.delete(id);
-      return;
-    }
-
-    const newSource = getSource(getState(), source.id);
-    if (!newSource) {
-      return;
-    }
-
-    if (!newSource.isWasm && isLoaded(newSource)) {
-      parser.setSource(newSource);
-      await dispatch(setBreakpointPositions(newSource.id));
-    }
-
-    // signal that the action is finished
-    deferred.resolve();
-    requests.delete(id);
-
-    return source;
+    return promise;
   };
 }