From 15815d0490cfc973740ef78d9b7847184e8c0c70 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 4 Apr 2024 06:30:26 -0700 Subject: [PATCH] Log cache I/O errors to `reporter` without killing the server Summary: TSIA Changelog: **[Fix]** Prevent cache write errors from killing the server, log them to `reporter` instead. Reviewed By: robhogan Differential Revision: D52043562 fbshipit-source-id: aed5216cf32b77cc9690ef3566329e10b03420f4 --- .../metro/src/DeltaBundler/Transformer.js | 18 +++++- .../__tests__/Transformer-test.js | 58 ++++++++++++++++++- packages/metro/src/lib/TerminalReporter.js | 32 +--------- packages/metro/src/lib/reporting.js | 8 +-- packages/metro/types/lib/reporting.d.ts | 8 +-- 5 files changed, 78 insertions(+), 46 deletions(-) diff --git a/packages/metro/src/DeltaBundler/Transformer.js b/packages/metro/src/DeltaBundler/Transformer.js index fe788627a8..e0ed9618d7 100644 --- a/packages/metro/src/DeltaBundler/Transformer.js +++ b/packages/metro/src/DeltaBundler/Transformer.js @@ -131,7 +131,16 @@ class Transformer { } let fullKey = Buffer.concat([partialKey, Buffer.from(sha1, 'hex')]); - const result = await cache.get(fullKey); + let result; + try { + result = await cache.get(fullKey); + } catch (error) { + this._config.reporter.update({ + type: 'cache_read_error', + error, + }); + throw error; + } // A valid result from the cache is used directly; otherwise we call into // the transformer to computed the corresponding result. @@ -154,7 +163,12 @@ class Transformer { } // Fire-and-forget cache set promise. - void cache.set(fullKey, data.result); + cache.set(fullKey, data.result).catch(error => { + this._config.reporter.update({ + type: 'cache_write_error', + error, + }); + }); return { ...data.result, diff --git a/packages/metro/src/DeltaBundler/__tests__/Transformer-test.js b/packages/metro/src/DeltaBundler/__tests__/Transformer-test.js index c5abc7348a..a91689b391 100644 --- a/packages/metro/src/DeltaBundler/__tests__/Transformer-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/Transformer-test.js @@ -101,11 +101,58 @@ describe('Transformer', function () { ); }); - it('short-circuits the transformer cache key when the cache is disabled', async () => { + it('logs cache read errors to reporter', async () => { + const readError = new Error('Cache write error'); + const get = jest.fn().mockImplementation(() => { + throw readError; + }); + const set = jest.fn(); + const mockReporter = { + update: jest.fn(), + }; + const transformerInstance = new Transformer( { ...commonOptions, - cacheStores: [], + reporter: mockReporter, + cacheStores: [{get, set}], + watchFolders, + }, + getSha1, + ); + + require('../WorkerFarm').prototype.transform.mockReturnValue({ + sha1: 'abcdefabcdefabcdefabcdefabcdefabcdefabcd', + result: {}, + }); + + await expect( + transformerInstance.transformFile('./foo.js', {}), + ).rejects.toBe(readError); + + expect(get).toHaveBeenCalledTimes(1); + + expect(mockReporter.update).toBeCalledWith({ + type: 'cache_read_error', + error: readError, + }); + }); + + it('logs cache write errors to reporter', async () => { + const get = jest.fn(); + const writeError = new Error('Cache write error'); + const set = jest.fn().mockImplementation(() => { + throw writeError; + }); + const mockReporter = { + update: jest.fn(), + }; + + const transformerInstance = new Transformer( + { + ...commonOptions, + reporter: mockReporter, + cacheStores: [{get, set}], watchFolders, }, getSha1, @@ -118,7 +165,12 @@ describe('Transformer', function () { await transformerInstance.transformFile('./foo.js', {}); - expect(require('../getTransformCacheKey')).not.toBeCalled(); + expect(set).toHaveBeenCalledTimes(1); + + expect(mockReporter.update).toBeCalledWith({ + type: 'cache_write_error', + error: writeError, + }); }); it('short-circuits the transformer cache key when the cache is disabled', async () => { diff --git a/packages/metro/src/lib/TerminalReporter.js b/packages/metro/src/lib/TerminalReporter.js index 20dadacf70..2cd1b9c9a3 100644 --- a/packages/metro/src/lib/TerminalReporter.js +++ b/packages/metro/src/lib/TerminalReporter.js @@ -11,11 +11,7 @@ 'use strict'; -import type { - BundleDetails, - GlobalCacheDisabledReason, - ReportableEvent, -} from './reporting'; +import type {BundleDetails, ReportableEvent} from './reporting'; import type {Terminal} from 'metro-core'; import type {HealthCheckResult, WatcherStatus} from 'metro-file-map'; @@ -53,9 +49,6 @@ type SnippetError = ErrnoError & snippet?: string, }; -const GLOBAL_CACHE_DISABLED_MESSAGE_FORMAT = - 'The global cache is now disabled because %s'; - const DARK_BLOCK_CHAR = '\u2593'; const LIGHT_BLOCK_CHAR = '\u2591'; const MAX_PROGRESS_BAR_CHAR_WIDTH = 16; @@ -141,26 +134,6 @@ class TerminalReporter { ); } - _logCacheDisabled(reason: GlobalCacheDisabledReason): void { - const format = GLOBAL_CACHE_DISABLED_MESSAGE_FORMAT; - switch (reason) { - case 'too_many_errors': - reporting.logWarning( - this.terminal, - format, - 'it has been failing too many times.', - ); - break; - case 'too_many_misses': - reporting.logWarning( - this.terminal, - format, - 'it has been missing too many consecutive keys.', - ); - break; - } - } - _logBundleBuildDone(buildID: string): void { const progress = this._activeBundles.get(buildID); if (progress != null) { @@ -253,9 +226,6 @@ class TerminalReporter { case 'bundling_error': this._logBundlingError(event.error); break; - case 'global_cache_disabled': - this._logCacheDisabled(event.reason); - break; case 'resolver_warning': this._logWarning(event.message); break; diff --git a/packages/metro/src/lib/reporting.js b/packages/metro/src/lib/reporting.js index f2432992d9..9971d41be1 100644 --- a/packages/metro/src/lib/reporting.js +++ b/packages/metro/src/lib/reporting.js @@ -20,8 +20,6 @@ const chalk = require('chalk'); const stripAnsi = require('strip-ansi'); const util = require('util'); -export type GlobalCacheDisabledReason = 'too_many_errors' | 'too_many_misses'; - export type BundleDetails = { bundleType: string, customResolverOptions: CustomResolverOptions, @@ -90,13 +88,13 @@ export type ReportableEvent = ... } | { - type: 'global_cache_error', + type: 'cache_read_error', error: Error, ... } | { - type: 'global_cache_disabled', - reason: GlobalCacheDisabledReason, + type: 'cache_write_error', + error: Error, ... } | {type: 'transform_cache_reset', ...} diff --git a/packages/metro/types/lib/reporting.d.ts b/packages/metro/types/lib/reporting.d.ts index df456fe6c5..b7d6e9f497 100644 --- a/packages/metro/types/lib/reporting.d.ts +++ b/packages/metro/types/lib/reporting.d.ts @@ -10,8 +10,6 @@ import type {HealthCheckResult, WatcherStatus} from 'metro-file-map'; -export type GlobalCacheDisabledReason = 'too_many_errors' | 'too_many_misses'; - export interface BundleDetails { bundleType: string; dev: boolean; @@ -65,12 +63,12 @@ export type ReportableEvent = totalFileCount: number; } | { - type: 'global_cache_error'; + type: 'cache_read_error'; error: Error; } | { - type: 'global_cache_disabled'; - reason: GlobalCacheDisabledReason; + type: 'cache_write_error'; + error: Error; } | {type: 'transform_cache_reset'} | {