Bug 1534847 - Part 1: Refactor loadSourceText to separate caching from logic. r=jlast
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 14 Mar 2019 14:34:59 -0400
changeset 522009 3befa22661a8
parent 522008 afd8d968a200
child 522010 35af4e6b5633
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 Summary: 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. Reviewers: jlast Reviewed By: jlast Bug #: 1534847 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;
   };
 }