Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suspend incomplete requests #9636

Open
wants to merge 8 commits into
base: v2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 62 additions & 1 deletion packages/core/core/src/RequestTracker.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -132,6 +133,7 @@ type Request<TInput, TResult> = {|
id: string,
+type: RequestType,
input: TInput,
incompleteRequest?: boolean,
run: ({|input: TInput, ...StaticRunOpts<TResult>|}) => Async<TResult>,
|};

Expand Down Expand Up @@ -1027,6 +1029,10 @@ export default class RequestTracker {
farm: WorkerFarm;
options: ParcelOptions;
signal: ?AbortSignal;
incompleteRequests: Map<
Copy link
Member

@devongovett devongovett Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have RequestGraph#incompleteNodeIds and also incompleteNodePromises. Could you use those here?

Request<mixed, mixed>['id'],
{|deferred: Deferred<void>, promise: Promise<void>, timerId: TimeoutID|},
> = new Map();

constructor({
graph,
Expand Down Expand Up @@ -1160,11 +1166,63 @@ export default class RequestTracker {
this.graph.replaceSubrequests(requestNodeId, subrequestContextKeys);
}

flushCachedRequest(requestId: ContentKey) {
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();
}
}

waitForFullRequest(requestId: ContentKey): Promise<void> {
let existingPromise = this.incompleteRequests.get(requestId);

if (existingPromise) {
return existingPromise.promise;
}

let deferredPromise = makeDeferredWithPromise();

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;
}

async runRequest<TInput, TResult>(
request: Request<TInput, TResult>,
opts?: ?RunRequestOpts,
): Promise<TResult> {
let requestId = this.graph.hasContentKey(request.id)
let hasKey = this.graph.hasContentKey(request.id);

if (
request.incompleteRequest &&
!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);
Expand Down Expand Up @@ -1223,6 +1281,9 @@ export default class RequestTracker {
deferred.resolve(false);
throw err;
} finally {
if (getFeatureFlag('devDepRequestBugFix')) {
this.flushCachedRequest(request.id);
}
this.graph.replaceSubrequests(requestNodeId, [...subRequestContentKeys]);
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/AssetGraphRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type AssetGraphRequest = {|
+type: typeof requestTypes.asset_graph_request,
run: RunInput => Async<AssetGraphRequestResult>,
input: AssetGraphRequestInput,
incompleteRequest?: boolean,
|};

export default function createAssetGraphRequest(
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/AssetRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export type AssetRequest = {|
+type: typeof requestTypes.asset_request,
run: (RunInput<AssetRequestResult>) => Async<AssetRequestResult>,
input: AssetRequestInput,
incompleteRequest?: boolean,
|};

export default function createAssetRequest(
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/BundleGraphRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ type BundleGraphRequest = {|
+type: typeof requestTypes.bundle_graph_request,
run: RunInput => Async<BundleGraphResult>,
input: BundleGraphRequestInput,
incompleteRequest?: boolean,
|};

export default function createBundleGraphRequest(
Expand Down
10 changes: 9 additions & 1 deletion packages/core/core/src/requests/DevDepRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,14 +190,22 @@ export async function runDevDepRequest<TResult>(
await api.runRequest<null, DevDepRequestResult | void>({
id: 'dev_dep_request:' + devDepRequest.specifier + ':' + devDepRequest.hash,
type: requestTypes.dev_dep_request,
incompleteRequest: !(
devDepRequest.invalidateOnFileChange &&
devDepRequest.invalidateOnFileCreate
),
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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably look at adding a lint rule to ensure nullthrows always has a description

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just change the type definition. It'd be a bit of work though 😅

devDepRequest.invalidateOnFileCreate,
'DevDepRequest missing invalidateOnFileCreate',
)) {
api.invalidateOnFileCreate(invalidation);
}
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/EntryRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export type EntryRequest = {|
+type: typeof requestTypes.entry_request,
run: (RunOpts<EntryResult>) => Async<EntryResult>,
input: ProjectPath,
incompleteRequest?: boolean,
|};

export type EntryResult = {|
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/PackageRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export type PackageRequest = {|
+type: typeof requestTypes.package_request,
run: (RunInput<BundleInfo>) => Async<BundleInfo>,
input: PackageRequestInput,
incompleteRequest?: boolean,
|};

export function createPackageRequest(
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/ParcelBuildRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export type ParcelBuildRequest = {|
+type: typeof requestTypes.parcel_build_request,
run: (RunInput<ParcelBuildRequestResult>) => Async<ParcelBuildRequestResult>,
input: ParcelBuildRequestInput,
incompleteRequest?: boolean,
|};

export default function createParcelBuildRequest(
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/ParcelConfigRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export type ParcelConfigRequest = {|
type: typeof requestTypes.parcel_config_request,
input: null,
run: (RunOpts<ConfigAndCachePath>) => Async<ConfigAndCachePath>,
incompleteRequest?: boolean,
|};

type ParcelConfigChain = {|
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/PathRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export type PathRequest = {|
+type: typeof requestTypes.path_request,
run: (RunOpts<?AssetGroup>) => Async<?AssetGroup>,
input: PathRequestInput,
incompleteRequest?: boolean,
|};

export type PathRequestInput = {|
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/TargetRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export type TargetRequest = {|
+type: typeof requestTypes.target_request,
run: (RunOpts<Array<Target>>) => Async<Array<Target>>,
input: Entry,
incompleteRequest?: boolean,
|};

const type = 'target_request';
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/ValidationRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type ValidationRequest = {|
+type: typeof requestTypes.validation_request,
run: (RunOpts<void>) => Async<void>,
input: ValidationRequestInput,
incompleteRequest?: boolean,
|};

type RunOpts<TResult> = {|
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/WriteBundleRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export type WriteBundleRequest = {|
+type: typeof requestTypes.write_bundle_request,
run: (RunInput<PackagedBundleInfo>) => Async<PackagedBundleInfo>,
input: WriteBundleRequestInput,
incompleteRequest?: boolean,
|};

/**
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/src/requests/WriteBundlesRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export type WriteBundlesRequest = {|
RunInput<Map<string, PackagedBundleInfo>>,
) => Async<Map<string, PackagedBundleInfo>>,
input: WriteBundlesRequestInput,
incompleteRequest?: boolean,
|};

/**
Expand Down
1 change: 1 addition & 0 deletions packages/core/core/test/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export const DEFAULT_OPTIONS: ParcelOptions = {
},
featureFlags: {
exampleFeature: false,
devDepRequestBugFix: false,
configKeyInvalidation: false,
},
};
Expand Down
1 change: 1 addition & 0 deletions packages/core/feature-flags/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
4 changes: 4 additions & 0 deletions packages/core/feature-flags/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
|};
3 changes: 3 additions & 0 deletions packages/core/integration-tests/test/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -1318,6 +1318,7 @@ describe('cache', function () {
let b = await testCache({
featureFlags: {
exampleFeature: false,
devDepRequestBugFix: false,
configKeyInvalidation: true,
},
async setup() {
Expand Down Expand Up @@ -1378,6 +1379,7 @@ describe('cache', function () {
let b = await testCache({
featureFlags: {
exampleFeature: false,
devDepRequestBugFix: false,
configKeyInvalidation: true,
},
async setup() {
Expand Down Expand Up @@ -1438,6 +1440,7 @@ describe('cache', function () {
let b = await testCache({
featureFlags: {
exampleFeature: false,
devDepRequestBugFix: false,
configKeyInvalidation: true,
},
async setup() {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/integration-tests/test/tracing.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
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 () {

Check failure on line 7 in packages/core/integration-tests/test/tracing.js

View workflow job for this annotation

GitHub Actions / Lint

Unexpected exclusive mocha test
await bundle(
path.join(__dirname, '/integration/typescript-jsx/index.tsx'),
{
Expand Down
2 changes: 1 addition & 1 deletion packages/core/integration-tests/test/transpilation.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
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 () {

Check failure on line 19 in packages/core/integration-tests/test/transpilation.js

View workflow job for this annotation

GitHub Actions / Lint

Unexpected exclusive mocha test
await bundle(path.join(__dirname, '/integration/babel-default/index.js'), {
defaultTargetOptions: {
engines: undefined,
Expand Down
Loading