packager: FBGlobalTransformCache: move the retry/error logic

Reviewed By: davidaurelio

Differential Revision: D4713585

fbshipit-source-id: 6a83f858692b8a1f6326051f3a3f4a3a549e4027

packages/metro-bundler/src/lib/GlobalTransformCache.js

@@ -12,6 +12,7 @@
 'use strict';
 
 const BatchProcessor = require('./BatchProcessor');
+const FetchError = require('node-fetch/lib/fetch-error');
 
 const crypto = require('crypto');
 const fetch = require('node-fetch');
@@ -21,9 +22,9 @@ const path = require('path');
 
 import type {Options as TransformOptions} from '../JSTransformer/worker/worker';
 import type {CachedResult, GetTransformCacheKey} from './TransformCache';
-import type {Reporter} from './reporting';
 
 type FetchResultURIs = (keys: Array<string>) => Promise<Map<string, string>>;
+type FetchResultFromURI = (uri: string) => Promise<?CachedResult>;
 type StoreResults = (resultsByKey: Map<string, CachedResult>) => Promise<void>;
 
 type FetchProps = {
@@ -43,7 +44,6 @@ class KeyURIFetcher {
 
   _batchProcessor: BatchProcessor<string, ?URI>;
   _fetchResultURIs: FetchResultURIs;
-  _processError: (error: Error) => mixed;
 
   /**
    * When a batch request fails for some reason, we process the error locally
@@ -51,13 +51,7 @@ class KeyURIFetcher {
    * a build will not fail just because of the cache.
    */
   async _processKeys(keys: Array<string>): Promise<Array<?URI>> {
-    let URIsByKey;
+    const URIsByKey = await this._fetchResultURIs(keys);
-    try {
-      URIsByKey = await this._fetchResultURIs(keys);
-    } catch (error) {
-      this._processError(error);
-      return new Array(keys.length);
-    }
     return keys.map(key => URIsByKey.get(key));
   }
 
@@ -65,14 +59,13 @@ class KeyURIFetcher {
     return await this._batchProcessor.queue(key);
   }
 
-  constructor(fetchResultURIs: FetchResultURIs, processError: (error: Error) => mixed) {
+  constructor(fetchResultURIs: FetchResultURIs) {
     this._fetchResultURIs = fetchResultURIs;
     this._batchProcessor = new BatchProcessor({
       maximumDelayMs: 10,
       maximumItems: 500,
       concurrency: 25,
     }, this._processKeys.bind(this));
-    this._processError = processError;
   }
 
 }
@@ -105,21 +98,6 @@ class KeyResultStore {
 
 }
 
-function validateCachedResult(cachedResult: mixed): ?CachedResult {
-  if (
-    cachedResult != null &&
-    typeof cachedResult === 'object' &&
-    typeof cachedResult.code === 'string' &&
-    Array.isArray(cachedResult.dependencies) &&
-    cachedResult.dependencies.every(dep => typeof dep === 'string') &&
-    Array.isArray(cachedResult.dependencyOffsets) &&
-    cachedResult.dependencyOffsets.every(offset => typeof offset === 'number')
-  ) {
-    return (cachedResult: any);
-  }
-  return undefined;
-}
-
 /**
  * The transform options contain absolute paths. This can contain, for
  * example, the username if someone works their home directory (very likely).
@@ -167,12 +145,30 @@ class TransformProfileSet {
   }
 }
 
+/**
+ * For some reason the result stored by the server for a key might mismatch what
+ * we expect a result to be. So we need to verify carefully the data.
+ */
+function validateCachedResult(cachedResult: mixed): ?CachedResult {
+  if (
+    cachedResult != null &&
+    typeof cachedResult === 'object' &&
+    typeof cachedResult.code === 'string' &&
+    Array.isArray(cachedResult.dependencies) &&
+    cachedResult.dependencies.every(dep => typeof dep === 'string') &&
+    Array.isArray(cachedResult.dependencyOffsets) &&
+    cachedResult.dependencyOffsets.every(offset => typeof offset === 'number')
+  ) {
+    return (cachedResult: any);
+  }
+  return null;
+}
+
 class GlobalTransformCache {
 
   _fetcher: KeyURIFetcher;
+  _fetchResultFromURI: FetchResultFromURI;
   _profileSet: TransformProfileSet;
-  _reporter: Reporter;
-  _retries: number;
   _store: ?KeyResultStore;
 
   /**
@@ -185,14 +181,13 @@ class GlobalTransformCache {
    */
   constructor(
     fetchResultURIs: FetchResultURIs,
+    fetchResultFromURI: FetchResultFromURI,
     storeResults: ?StoreResults,
     profiles: Iterable<TransformProfile>,
-    reporter: Reporter,
   ) {
-    this._fetcher = new KeyURIFetcher(fetchResultURIs, this._processError.bind(this));
+    this._fetcher = new KeyURIFetcher(fetchResultURIs);
     this._profileSet = new TransformProfileSet(profiles);
-    this._reporter = reporter;
+    this._fetchResultFromURI = fetchResultFromURI;
-    this._retries = 4;
     if (storeResults != null) {
       this._store = new KeyResultStore(storeResults);
     }
@@ -211,26 +206,12 @@ class GlobalTransformCache {
     return `${digest}-${path.basename(props.filePath)}`;
   }
 
-  /**
-   * If too many errors already happened, we just drop the additional errors.
-   */
-  _processError(error: Error) {
-    if (this._retries <= 0) {
-      return;
-    }
-    this._reporter.update({type: 'global_cache_error', error});
-    --this._retries;
-    if (this._retries <= 0) {
-      this._reporter.update({type: 'global_cache_disabled', reason: 'too_many_errors'});
-    }
-  }
-
   /**
    * We may want to improve that logic to return a stream instead of the whole
    * blob of transformed results. However the results are generally only a few
    * megabytes each.
    */
-  async _fetchFromURI(uri: string): Promise<CachedResult> {
+  static async _fetchResultFromURI(uri: string): Promise<CachedResult> {
     const response = await fetch(uri, {method: 'GET', timeout: 8000});
     if (response.status !== 200) {
       throw new Error(`Unexpected HTTP status: ${response.status} ${response.statusText} `);
@@ -244,16 +225,16 @@ class GlobalTransformCache {
   }
 
   /**
-   * Wrap `_fetchFromURI` with error logging, and return an empty result instead
+   * It happens from time to time that a fetch timeouts, we want to try these
-   * of errors. This is because the global cache is not critical to the normal
+   * again a second time.
-   * packager operation.
    */
-  async _tryFetchingFromURI(uri: string): Promise<?CachedResult> {
+  static fetchResultFromURI(uri: string): Promise<CachedResult> {
-    try {
+    return GlobalTransformCache._fetchResultFromURI(uri).catch(error => {
-      return await this._fetchFromURI(uri);
+      if (!(error instanceof FetchError && error.type === 'request-timeout')) {
-    } catch (error) {
+        throw error;
-      this._processError(error);
+      }
-    }
+      return this._fetchResultFromURI(uri);
+    });
   }
 
   /**
@@ -261,14 +242,14 @@ class GlobalTransformCache {
    * key yet, or an error happened, processed separately.
    */
   async fetch(props: FetchProps): Promise<?CachedResult> {
-    if (this._retries <= 0 || !this._profileSet.has(props.transformOptions)) {
+    if (!this._profileSet.has(props.transformOptions)) {
       return null;
     }
     const uri = await this._fetcher.fetch(GlobalTransformCache.keyOf(props));
     if (uri == null) {
       return null;
     }
-    return await this._tryFetchingFromURI(uri);
+    return await this._fetchResultFromURI(uri);
   }
 
   store(props: FetchProps, result: CachedResult) {