From e3274bdef353f5d629e93f66fe849ff815a9adfa Mon Sep 17 00:00:00 2001 From: Jean Lauliac Date: Thu, 16 Mar 2017 13:47:25 -0700 Subject: [PATCH] packager: FBGlobalTransformCache: move the retry/error logic Reviewed By: davidaurelio Differential Revision: D4713585 fbshipit-source-id: 6a83f858692b8a1f6326051f3a3f4a3a549e4027 --- .../src/lib/GlobalTransformCache.js | 97 ++++++++----------- 1 file changed, 39 insertions(+), 58 deletions(-) diff --git a/packages/metro-bundler/src/lib/GlobalTransformCache.js b/packages/metro-bundler/src/lib/GlobalTransformCache.js index 002960df..b74a1981 100644 --- a/packages/metro-bundler/src/lib/GlobalTransformCache.js +++ b/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) => Promise>; +type FetchResultFromURI = (uri: string) => Promise; type StoreResults = (resultsByKey: Map) => Promise; type FetchProps = { @@ -43,7 +44,6 @@ class KeyURIFetcher { _batchProcessor: BatchProcessor; _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): Promise> { - let URIsByKey; - try { - URIsByKey = await this._fetchResultURIs(keys); - } catch (error) { - this._processError(error); - return new Array(keys.length); - } + const URIsByKey = await this._fetchResultURIs(keys); 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, - 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._retries = 4; + this._fetchResultFromURI = fetchResultFromURI; 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 { + static async _fetchResultFromURI(uri: string): Promise { 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 - * of errors. This is because the global cache is not critical to the normal - * packager operation. + * It happens from time to time that a fetch timeouts, we want to try these + * again a second time. */ - async _tryFetchingFromURI(uri: string): Promise { - try { - return await this._fetchFromURI(uri); - } catch (error) { - this._processError(error); - } + static fetchResultFromURI(uri: string): Promise { + return GlobalTransformCache._fetchResultFromURI(uri).catch(error => { + if (!(error instanceof FetchError && error.type === 'request-timeout')) { + throw error; + } + return this._fetchResultFromURI(uri); + }); } /** @@ -261,14 +242,14 @@ class GlobalTransformCache { * key yet, or an error happened, processed separately. */ async fetch(props: FetchProps): Promise { - 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) {