From fd4a4c19c284ed38b3415e5ead1667235ceb9ed0 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 12 Apr 2024 10:05:22 +1000 Subject: [PATCH 1/7] Add a feature flag for the devdep fix --- packages/core/feature-flags/src/index.js | 1 + packages/core/feature-flags/src/types.js | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/packages/core/feature-flags/src/index.js b/packages/core/feature-flags/src/index.js index c66b372d0dd..e8b65cea35d 100644 --- a/packages/core/feature-flags/src/index.js +++ b/packages/core/feature-flags/src/index.js @@ -8,6 +8,7 @@ export type FeatureFlags = _FeatureFlags; export const DEFAULT_FEATURE_FLAGS: FeatureFlags = { exampleFeature: false, configKeyInvalidation: false, + devDepRequestBugFix: false, }; let featureFlagValues: FeatureFlags = {...DEFAULT_FEATURE_FLAGS}; diff --git a/packages/core/feature-flags/src/types.js b/packages/core/feature-flags/src/types.js index 4edb1cf42f0..158aa4b3f67 100644 --- a/packages/core/feature-flags/src/types.js +++ b/packages/core/feature-flags/src/types.js @@ -9,4 +9,8 @@ export type FeatureFlags = {| * `config.getConfigFrom(..., {packageKey: '...'})` and the value itself hasn't changed. */ +configKeyInvalidation: boolean, + /** + * Enable experimental fix for the DevDepRequest build cache bug + */ + +devDepRequestBugFix: boolean, |}; From 074ae298c4e420c8e30c191e227191d69c0bc771 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 12 Apr 2024 10:05:50 +1000 Subject: [PATCH 2/7] Add some proper messages to the devdep nullthrows --- packages/core/core/src/requests/DevDepRequest.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/core/core/src/requests/DevDepRequest.js b/packages/core/core/src/requests/DevDepRequest.js index eb1d93d5aa4..1d0be3e2f1f 100644 --- a/packages/core/core/src/requests/DevDepRequest.js +++ b/packages/core/core/src/requests/DevDepRequest.js @@ -191,13 +191,17 @@ export async function runDevDepRequest( id: 'dev_dep_request:' + devDepRequest.specifier + ':' + devDepRequest.hash, type: requestTypes.dev_dep_request, run: ({api}) => { - for (let filePath of nullthrows(devDepRequest.invalidateOnFileChange)) { + for (let filePath of nullthrows( + devDepRequest.invalidateOnFileChange, + 'DevDepRequest missing invalidateOnFileChange', + )) { api.invalidateOnFileUpdate(filePath); api.invalidateOnFileDelete(filePath); } for (let invalidation of nullthrows( devDepRequest.invalidateOnFileCreate, + 'DevDepRequest missing invalidateOnFileCreate', )) { api.invalidateOnFileCreate(invalidation); } From db1ecfd16af97e6cf2c0a1552181915c881687bb Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 12 Apr 2024 10:07:39 +1000 Subject: [PATCH 3/7] Suspend incomplete requests --- packages/core/core/src/RequestTracker.js | 48 ++++++++++++++++++- .../core/core/src/requests/DevDepRequest.js | 4 ++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/packages/core/core/src/RequestTracker.js b/packages/core/core/src/RequestTracker.js index 623fcbe864b..22e1ed54304 100644 --- a/packages/core/core/src/RequestTracker.js +++ b/packages/core/core/src/RequestTracker.js @@ -54,6 +54,7 @@ import {report} from './ReporterRunner'; import {PromiseQueue} from '@parcel/utils'; import type {Cache} from '@parcel/cache'; import {getConfigKeyContentHash} from './requests/ConfigRequest'; +import {getFeatureFlag} from '@parcel/feature-flags'; export const requestGraphEdgeTypes = { subrequest: 2, @@ -132,6 +133,7 @@ type Request = {| id: string, +type: RequestType, input: TInput, + ensureCache?: boolean, run: ({|input: TInput, ...StaticRunOpts|}) => Async, |}; @@ -1027,6 +1029,10 @@ export default class RequestTracker { farm: WorkerFarm; options: ParcelOptions; signal: ?AbortSignal; + incompleteRequests: Map< + Request['id'], + {|deferred: Deferred, promise: Promise, timerId: TimeoutID|}, + > = new Map(); constructor({ graph, @@ -1160,11 +1166,48 @@ export default class RequestTracker { this.graph.replaceSubrequests(requestNodeId, subrequestContextKeys); } + flushCachedRequest(requestId: ContentKey) { + let existingPromise = this.incompleteRequests.get(requestId); + + if (existingPromise) { + clearTimeout(existingPromise.timerId); + existingPromise.deferred.resolve(); + } + } + + waitForFullRequest(requestId: ContentKey): Promise { + let existingPromise = this.incompleteRequests.get(requestId); + + if (existingPromise) { + return existingPromise.promise; + } + + let deferredPromise = makeDeferredWithPromise(); + let timerId = setTimeout(() => deferredPromise.deferred.resolve(), 5_000); + this.incompleteRequests.set(requestId, {...deferredPromise, timerId}); + + return deferredPromise.promise; + } + async runRequest( request: Request, opts?: ?RunRequestOpts, ): Promise { - let requestId = this.graph.hasContentKey(request.id) + let hasKey = this.graph.hasContentKey(request.id); + + if ( + request.ensureCache && + !hasKey && + getFeatureFlag('devDepRequestBugFix') + ) { + // To avoid possible race conditions with the worker farm build cache, we + // suspend processing of an incomplete request until the full details are back + // from another worker. + await this.waitForFullRequest(request.id); + hasKey = this.graph.hasContentKey(request.id); + } + + let requestId = hasKey ? this.graph.getNodeIdByContentKey(request.id) : undefined; let hasValidResult = requestId != null && this.hasValidResult(requestId); @@ -1223,6 +1266,9 @@ export default class RequestTracker { deferred.resolve(false); throw err; } finally { + if (getFeatureFlag('devDepRequestBugFix')) { + this.flushCachedRequest(request.id); + } this.graph.replaceSubrequests(requestNodeId, [...subRequestContentKeys]); } } diff --git a/packages/core/core/src/requests/DevDepRequest.js b/packages/core/core/src/requests/DevDepRequest.js index 1d0be3e2f1f..fb63838e405 100644 --- a/packages/core/core/src/requests/DevDepRequest.js +++ b/packages/core/core/src/requests/DevDepRequest.js @@ -190,6 +190,10 @@ export async function runDevDepRequest( await api.runRequest({ id: 'dev_dep_request:' + devDepRequest.specifier + ':' + devDepRequest.hash, type: requestTypes.dev_dep_request, + ensureCache: !( + devDepRequest.invalidateOnFileChange && + devDepRequest.invalidateOnFileCreate + ), run: ({api}) => { for (let filePath of nullthrows( devDepRequest.invalidateOnFileChange, From 947f94ae3e4590581a93a45d015dbd7a121e0d76 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 12 Apr 2024 10:21:52 +1000 Subject: [PATCH 4/7] Rename property to align with cache --- packages/core/core/src/RequestTracker.js | 2 +- packages/core/core/src/requests/DevDepRequest.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/core/src/RequestTracker.js b/packages/core/core/src/RequestTracker.js index 22e1ed54304..2d4448aeeeb 100644 --- a/packages/core/core/src/RequestTracker.js +++ b/packages/core/core/src/RequestTracker.js @@ -133,7 +133,7 @@ type Request = {| id: string, +type: RequestType, input: TInput, - ensureCache?: boolean, + incompleteRequest?: boolean, run: ({|input: TInput, ...StaticRunOpts|}) => Async, |}; diff --git a/packages/core/core/src/requests/DevDepRequest.js b/packages/core/core/src/requests/DevDepRequest.js index fb63838e405..5f95e48160b 100644 --- a/packages/core/core/src/requests/DevDepRequest.js +++ b/packages/core/core/src/requests/DevDepRequest.js @@ -190,7 +190,7 @@ export async function runDevDepRequest( await api.runRequest({ id: 'dev_dep_request:' + devDepRequest.specifier + ':' + devDepRequest.hash, type: requestTypes.dev_dep_request, - ensureCache: !( + incompleteRequest: !( devDepRequest.invalidateOnFileChange && devDepRequest.invalidateOnFileCreate ), From 0b614e93edcf6e64d2b63e3a6f0f0cc7a82bc285 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 12 Apr 2024 10:21:59 +1000 Subject: [PATCH 5/7] Add some logging --- packages/core/core/src/RequestTracker.js | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/core/core/src/RequestTracker.js b/packages/core/core/src/RequestTracker.js index 2d4448aeeeb..5b322bd068e 100644 --- a/packages/core/core/src/RequestTracker.js +++ b/packages/core/core/src/RequestTracker.js @@ -1170,6 +1170,12 @@ export default class RequestTracker { let existingPromise = this.incompleteRequests.get(requestId); if (existingPromise) { + logger.verbose({ + origin: '@parcel/core', + message: 'Incomplete request promise resolved', + meta: {trackableEvent: 'incomplete_promise_resolved'}, + }); + clearTimeout(existingPromise.timerId); existingPromise.deferred.resolve(); } @@ -1183,7 +1189,16 @@ export default class RequestTracker { } let deferredPromise = makeDeferredWithPromise(); - let timerId = setTimeout(() => deferredPromise.deferred.resolve(), 5_000); + + let timerId = setTimeout(() => { + logger.verbose({ + origin: '@parcel/core', + message: 'Incomplete request promise timed out', + meta: {trackableEvent: 'incomplete_promise_timeout'}, + }); + deferredPromise.deferred.resolve(); + }, 5_000); + this.incompleteRequests.set(requestId, {...deferredPromise, timerId}); return deferredPromise.promise; @@ -1196,7 +1211,7 @@ export default class RequestTracker { let hasKey = this.graph.hasContentKey(request.id); if ( - request.ensureCache && + request.incompleteRequest && !hasKey && getFeatureFlag('devDepRequestBugFix') ) { From b2a31f8ddf9108d48d1c8379605a5b3a04f7ef07 Mon Sep 17 00:00:00 2001 From: mattcompiles Date: Fri, 12 Apr 2024 14:04:28 +1000 Subject: [PATCH 6/7] Fix flow errors --- packages/core/core/src/requests/AssetGraphRequest.js | 1 + packages/core/core/src/requests/AssetRequest.js | 1 + packages/core/core/src/requests/BundleGraphRequest.js | 1 + packages/core/core/src/requests/EntryRequest.js | 1 + packages/core/core/src/requests/PackageRequest.js | 1 + packages/core/core/src/requests/ParcelBuildRequest.js | 1 + packages/core/core/src/requests/ParcelConfigRequest.js | 1 + packages/core/core/src/requests/PathRequest.js | 1 + packages/core/core/src/requests/TargetRequest.js | 1 + packages/core/core/src/requests/ValidationRequest.js | 1 + packages/core/core/src/requests/WriteBundleRequest.js | 1 + packages/core/core/src/requests/WriteBundlesRequest.js | 1 + packages/core/core/test/test-utils.js | 1 + packages/core/integration-tests/test/cache.js | 3 +++ packages/core/integration-tests/test/tracing.js | 2 +- packages/core/integration-tests/test/transpilation.js | 2 +- 16 files changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/core/core/src/requests/AssetGraphRequest.js b/packages/core/core/src/requests/AssetGraphRequest.js index 98c601faaad..37600d6f3b3 100644 --- a/packages/core/core/src/requests/AssetGraphRequest.js +++ b/packages/core/core/src/requests/AssetGraphRequest.js @@ -66,6 +66,7 @@ type AssetGraphRequest = {| +type: typeof requestTypes.asset_graph_request, run: RunInput => Async, input: AssetGraphRequestInput, + incompleteRequest?: boolean, |}; export default function createAssetGraphRequest( diff --git a/packages/core/core/src/requests/AssetRequest.js b/packages/core/core/src/requests/AssetRequest.js index e2187e2cc4a..df86aaf95ad 100644 --- a/packages/core/core/src/requests/AssetRequest.js +++ b/packages/core/core/src/requests/AssetRequest.js @@ -32,6 +32,7 @@ export type AssetRequest = {| +type: typeof requestTypes.asset_request, run: (RunInput) => Async, input: AssetRequestInput, + incompleteRequest?: boolean, |}; export default function createAssetRequest( diff --git a/packages/core/core/src/requests/BundleGraphRequest.js b/packages/core/core/src/requests/BundleGraphRequest.js index 466f76a1a1c..b36354e61c9 100644 --- a/packages/core/core/src/requests/BundleGraphRequest.js +++ b/packages/core/core/src/requests/BundleGraphRequest.js @@ -82,6 +82,7 @@ type BundleGraphRequest = {| +type: typeof requestTypes.bundle_graph_request, run: RunInput => Async, input: BundleGraphRequestInput, + incompleteRequest?: boolean, |}; export default function createBundleGraphRequest( diff --git a/packages/core/core/src/requests/EntryRequest.js b/packages/core/core/src/requests/EntryRequest.js index 149b65d0d9a..0aa9a32bf7e 100644 --- a/packages/core/core/src/requests/EntryRequest.js +++ b/packages/core/core/src/requests/EntryRequest.js @@ -36,6 +36,7 @@ export type EntryRequest = {| +type: typeof requestTypes.entry_request, run: (RunOpts) => Async, input: ProjectPath, + incompleteRequest?: boolean, |}; export type EntryResult = {| diff --git a/packages/core/core/src/requests/PackageRequest.js b/packages/core/core/src/requests/PackageRequest.js index f83da83dbd4..dd6ae60e713 100644 --- a/packages/core/core/src/requests/PackageRequest.js +++ b/packages/core/core/src/requests/PackageRequest.js @@ -34,6 +34,7 @@ export type PackageRequest = {| +type: typeof requestTypes.package_request, run: (RunInput) => Async, input: PackageRequestInput, + incompleteRequest?: boolean, |}; export function createPackageRequest( diff --git a/packages/core/core/src/requests/ParcelBuildRequest.js b/packages/core/core/src/requests/ParcelBuildRequest.js index b3057590783..9ada1c44727 100644 --- a/packages/core/core/src/requests/ParcelBuildRequest.js +++ b/packages/core/core/src/requests/ParcelBuildRequest.js @@ -46,6 +46,7 @@ export type ParcelBuildRequest = {| +type: typeof requestTypes.parcel_build_request, run: (RunInput) => Async, input: ParcelBuildRequestInput, + incompleteRequest?: boolean, |}; export default function createParcelBuildRequest( diff --git a/packages/core/core/src/requests/ParcelConfigRequest.js b/packages/core/core/src/requests/ParcelConfigRequest.js index bb521c209f3..4cd714bf443 100644 --- a/packages/core/core/src/requests/ParcelConfigRequest.js +++ b/packages/core/core/src/requests/ParcelConfigRequest.js @@ -56,6 +56,7 @@ export type ParcelConfigRequest = {| type: typeof requestTypes.parcel_config_request, input: null, run: (RunOpts) => Async, + incompleteRequest?: boolean, |}; type ParcelConfigChain = {| diff --git a/packages/core/core/src/requests/PathRequest.js b/packages/core/core/src/requests/PathRequest.js index 5917443e01d..73fbb775d05 100644 --- a/packages/core/core/src/requests/PathRequest.js +++ b/packages/core/core/src/requests/PathRequest.js @@ -58,6 +58,7 @@ export type PathRequest = {| +type: typeof requestTypes.path_request, run: (RunOpts) => Async, input: PathRequestInput, + incompleteRequest?: boolean, |}; export type PathRequestInput = {| diff --git a/packages/core/core/src/requests/TargetRequest.js b/packages/core/core/src/requests/TargetRequest.js index 35f1171e11a..0694bb69f09 100644 --- a/packages/core/core/src/requests/TargetRequest.js +++ b/packages/core/core/src/requests/TargetRequest.js @@ -93,6 +93,7 @@ export type TargetRequest = {| +type: typeof requestTypes.target_request, run: (RunOpts>) => Async>, input: Entry, + incompleteRequest?: boolean, |}; const type = 'target_request'; diff --git a/packages/core/core/src/requests/ValidationRequest.js b/packages/core/core/src/requests/ValidationRequest.js index 307306caa1a..3a3a26116f7 100644 --- a/packages/core/core/src/requests/ValidationRequest.js +++ b/packages/core/core/src/requests/ValidationRequest.js @@ -17,6 +17,7 @@ type ValidationRequest = {| +type: typeof requestTypes.validation_request, run: (RunOpts) => Async, input: ValidationRequestInput, + incompleteRequest?: boolean, |}; type RunOpts = {| diff --git a/packages/core/core/src/requests/WriteBundleRequest.js b/packages/core/core/src/requests/WriteBundleRequest.js index 1b8266cd995..629ae809591 100644 --- a/packages/core/core/src/requests/WriteBundleRequest.js +++ b/packages/core/core/src/requests/WriteBundleRequest.js @@ -61,6 +61,7 @@ export type WriteBundleRequest = {| +type: typeof requestTypes.write_bundle_request, run: (RunInput) => Async, input: WriteBundleRequestInput, + incompleteRequest?: boolean, |}; /** diff --git a/packages/core/core/src/requests/WriteBundlesRequest.js b/packages/core/core/src/requests/WriteBundlesRequest.js index 1559872e972..6ac32107da1 100644 --- a/packages/core/core/src/requests/WriteBundlesRequest.js +++ b/packages/core/core/src/requests/WriteBundlesRequest.js @@ -33,6 +33,7 @@ export type WriteBundlesRequest = {| RunInput>, ) => Async>, input: WriteBundlesRequestInput, + incompleteRequest?: boolean, |}; /** diff --git a/packages/core/core/test/test-utils.js b/packages/core/core/test/test-utils.js index b44e22be99f..7e8e1652d9d 100644 --- a/packages/core/core/test/test-utils.js +++ b/packages/core/core/test/test-utils.js @@ -53,6 +53,7 @@ export const DEFAULT_OPTIONS: ParcelOptions = { }, featureFlags: { exampleFeature: false, + devDepRequestBugFix: false, configKeyInvalidation: false, }, }; diff --git a/packages/core/integration-tests/test/cache.js b/packages/core/integration-tests/test/cache.js index f6a8bf07180..91495d98397 100644 --- a/packages/core/integration-tests/test/cache.js +++ b/packages/core/integration-tests/test/cache.js @@ -1318,6 +1318,7 @@ describe('cache', function () { let b = await testCache({ featureFlags: { exampleFeature: false, + devDepRequestBugFix: false, configKeyInvalidation: true, }, async setup() { @@ -1378,6 +1379,7 @@ describe('cache', function () { let b = await testCache({ featureFlags: { exampleFeature: false, + devDepRequestBugFix: false, configKeyInvalidation: true, }, async setup() { @@ -1438,6 +1440,7 @@ describe('cache', function () { let b = await testCache({ featureFlags: { exampleFeature: false, + devDepRequestBugFix: false, configKeyInvalidation: true, }, async setup() { diff --git a/packages/core/integration-tests/test/tracing.js b/packages/core/integration-tests/test/tracing.js index 1c9a9b72493..5d092b14664 100644 --- a/packages/core/integration-tests/test/tracing.js +++ b/packages/core/integration-tests/test/tracing.js @@ -4,7 +4,7 @@ import path from 'path'; import {distDir, bundle, outputFS} from '@parcel/test-utils'; describe('tracing', function () { - it('should produce a trace', async function () { + it.only('should produce a trace', async function () { await bundle( path.join(__dirname, '/integration/typescript-jsx/index.tsx'), { diff --git a/packages/core/integration-tests/test/transpilation.js b/packages/core/integration-tests/test/transpilation.js index a09ffe3906b..9fa85dd66c6 100644 --- a/packages/core/integration-tests/test/transpilation.js +++ b/packages/core/integration-tests/test/transpilation.js @@ -16,7 +16,7 @@ import nullthrows from 'nullthrows'; const inputDir = path.join(__dirname, '/input'); describe('transpilation', function () { - it('should not transpile if no targets are defined', async function () { + it.only('should not transpile if no targets are defined', async function () { await bundle(path.join(__dirname, '/integration/babel-default/index.js'), { defaultTargetOptions: { engines: undefined, From 26b48e9b5ed378e7bf1a04ac21635070cad04fc5 Mon Sep 17 00:00:00 2001 From: Ben Jervis Date: Fri, 12 Apr 2024 16:59:04 +1000 Subject: [PATCH 7/7] Remove some `.only`s from tests --- packages/core/integration-tests/test/tracing.js | 2 +- packages/core/integration-tests/test/transpilation.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/integration-tests/test/tracing.js b/packages/core/integration-tests/test/tracing.js index 5d092b14664..1c9a9b72493 100644 --- a/packages/core/integration-tests/test/tracing.js +++ b/packages/core/integration-tests/test/tracing.js @@ -4,7 +4,7 @@ import path from 'path'; import {distDir, bundle, outputFS} from '@parcel/test-utils'; describe('tracing', function () { - it.only('should produce a trace', async function () { + it('should produce a trace', async function () { await bundle( path.join(__dirname, '/integration/typescript-jsx/index.tsx'), { diff --git a/packages/core/integration-tests/test/transpilation.js b/packages/core/integration-tests/test/transpilation.js index 9fa85dd66c6..a09ffe3906b 100644 --- a/packages/core/integration-tests/test/transpilation.js +++ b/packages/core/integration-tests/test/transpilation.js @@ -16,7 +16,7 @@ import nullthrows from 'nullthrows'; const inputDir = path.join(__dirname, '/input'); describe('transpilation', function () { - it.only('should not transpile if no targets are defined', async function () { + it('should not transpile if no targets are defined', async function () { await bundle(path.join(__dirname, '/integration/babel-default/index.js'), { defaultTargetOptions: { engines: undefined,