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

fix: add a temporary property '_isRootFile_' for each promise result of cache to prevent i… #165

Merged
merged 2 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/two-feet-heal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'tsconfck': patch
---

fix deadlock when referenced tsconfig extends original
3 changes: 3 additions & 0 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -308,5 +308,8 @@ export class TSConfckCache<T> {
* @throws {unknown} if cached value is an error
*/
getParseResult(file: string): Promise<T> | T;
/**
* @param isRootFile a flag to check if current file which involking the parse() api, used to distinguish the normal cache which only parsed by parseFile()
* */
}
```
10 changes: 9 additions & 1 deletion packages/tsconfck/src/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,17 @@ export class TSConfckCache {
* @internal
* @private
* @param file
* @param {boolean} isRootFile a flag to check if current file which involking the parse() api, used to distinguish the normal cache which only parsed by parseFile()
* @param {Promise<T>} result
*/
setParseResult(file, result) {
setParseResult(file, result, isRootFile = false) {
// _isRootFile_ is a temporary property for Promise result, used to prevent deadlock with cache
Object.defineProperty(result, '_isRootFile_', {
value: isRootFile,
writable: false,
enumerable: false,
configurable: false
});
this.#parsed.set(file, result);
result
.then((parsed) => {
Expand Down
15 changes: 11 additions & 4 deletions packages/tsconfck/src/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export async function parse(filename, options) {
/** @type {Promise<import('./public.d.ts').TSConfckParseResult>}*/
promise
} = makePromise();
cache?.setParseResult(filename, promise);
cache?.setParseResult(filename, promise, true);
try {
let tsconfigFile =
(await resolveTSConfigJson(filename, cache)) || (await find(filename, options));
Expand Down Expand Up @@ -76,7 +76,7 @@ async function getParsedDeep(filename, cache, options) {
parseExtends(result, cache),
parseReferences(result, options)
]).then(() => result);
cache.setParseResult(filename, promise);
cache.setParseResult(filename, promise, true);
return promise;
}
return result;
Expand All @@ -90,7 +90,11 @@ async function getParsedDeep(filename, cache, options) {
* @returns {Promise<import('./public.d.ts').TSConfckParseResult>}
*/
async function parseFile(tsconfigFile, cache, skipCache) {
if (!skipCache && cache?.hasParseResult(tsconfigFile)) {
if (
!skipCache &&
cache?.hasParseResult(tsconfigFile) &&
!cache.getParseResult(tsconfigFile)._isRootFile_
) {
return cache.getParseResult(tsconfigFile);
}
const promise = fs
Expand All @@ -112,7 +116,10 @@ async function parseFile(tsconfigFile, cache, skipCache) {
e
);
});
if (!skipCache) {
if (
!skipCache &&
(!cache?.hasParseResult(tsconfigFile) || !cache.getParseResult(tsconfigFile)._isRootFile_)
) {
cache?.setParseResult(tsconfigFile, promise);
}
return promise;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function foo() {
return 'foo';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"include": ["./foo.ts"],
"extends": "../tsconfig",
"compilerOptions": {
"composite": true,
"strict": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { foo } from '../src/foo';
import * as assert from 'assert';

function test() {
const actual = foo();
const expected = 'foo';
assert.strictEqual(actual, expected);
}
test();
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"include": ["./*.test.ts"],
"extends": "../tsconfig",
"compilerOptions": {
"composite": true,
"strict": false
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"references": [
{"path": "./src/tsconfig.src.json"},
{"path": "./tests/tsconfig.test.json"}
]
}
94 changes: 70 additions & 24 deletions packages/tsconfck/tests/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { promises as fs } from 'node:fs';
import { transform as esbuildTransform } from 'esbuild';
import ts from 'typescript';
import { TSConfckCache } from '../src/cache.js';
import { posix2native } from '../src/util.js';

describe('parse', () => {
it('should be a function', () => {
Expand Down Expand Up @@ -111,21 +110,42 @@ describe('parse', () => {
});

it('should work with cache', async () => {
async function derivedFilesShouldBeCached(files, type) {
for (const file of files) {
expect(cache.hasParseResult(file), `cache exists for ${type} tsconfig ${file}`).toBe(true);
const parsed = await parse(file);
expect(parsed, `parsing ${type} tsconfig ${file} worked`).toBeTruthy();
if (parsed.tsconfig[type]) {
expect(
parsed.tsconfig[type].length,
`nested ${type} has been resolved for ${file}`
).toBeGreaterThanOrEqual(1);
}
}
}

async function extendsShouldBeCached(actual) {
await derivedFilesShouldBeCached(
actual.extended.map((e) => e.tsconfigFile),
'extends'
);
}

async function referencesShouldBeCached(actual) {
await derivedFilesShouldBeCached(
actual.referenced.map((e) => e.tsconfigFile),
'references'
);
}

// use the more interesting samples with extensions and solution-style
const samples = [
...(await globFixtures('parse/valid/with_extends/**/tsconfig.json')),
...(await globFixtures('parse/solution/**/*.ts'))
...(await globFixtures('parse/solution/**/*.ts')),
...(await globFixtures('parse/solution/**/*.json'))
];
const cache = new TSConfckCache();
for (const filename of samples) {
// these 3 directories have extends declarations that lead to the configs being parsed from extends first and cached before explicit access
const expectedHasParseResult = ['dotdot', 'nested', 'nested/src'].some((dir) =>
filename.endsWith(posix2native(`with_extends/${dir}/tsconfig.json`))
);
expect(
cache.hasParseResult(filename),
`cache does ${!expectedHasParseResult ? 'not ' : ' '}exist for ${filename}`
).toBe(expectedHasParseResult);
const actual = await parse(filename, { cache });
await expectToMatchSnap(
actual.tsconfig,
Expand All @@ -139,21 +159,13 @@ describe('parse', () => {
actual.tsconfig
);
if (actual.extended) {
for (const extended of actual.extended.map((e) => e.tsconfigFile)) {
expect(
cache.hasParseResult(extended),
`cache exists for extended tsconfig ${extended}`
).toBe(true);
const parsedExtended = await parse(extended);
expect(parsedExtended, `parsing extended ${extended} worked`).toBeTruthy();
if (parsedExtended.tsconfig.extends) {
expect(
parsedExtended.extended.length,
`nested extends has been resolved for ${extended}`
).toBeGreaterThanOrEqual(1);
}
}
await extendsShouldBeCached(actual);
}

if (actual.referenced) {
await referencesShouldBeCached(actual);
}

const reparsedResult = await parse(filename, { cache });
expect(reparsedResult, `reparsedResult was returned from cache for ${filename}`).toBe(cached);

Expand Down Expand Up @@ -189,6 +201,40 @@ describe('parse', () => {
}
});

it('should not lock up parsing in parallel with cache', async () => {
const samples = [
...(await globFixtures('parse/valid/**/*.ts')),
...(await globFixtures('parse/valid/**/*.json')),
...(await globFixtures('parse/solution/**/*.ts')),
...(await globFixtures('parse/solution/**/*.json'))
];
expect(samples.length).toBeGreaterThan(10);
const cache = new TSConfckCache();
const unfinished = new Set(samples);
try {
const results = await Promise.race([
Promise.all(
samples.map((s) => {
const sample = s;
return parse(sample, { cache }).then((result) => {
unfinished.delete(sample);
// the result should not include the temporary property "__isRootFile__"
expect(result._isRootFile_).toBeUndefined();
});
})
),
new Promise((_, reject) => setTimeout(reject, 500))
]);
expect(results.length).toBe(samples.length);
} catch (e) {
expect.fail(
`did not process all files, some of these are blocking each other:\n${[...unfinished].join(
'\n'
)}\n`
);
}
});

it('should resolve with tsconfig that is isomorphic', async () => {
const tempDir = await copyFixtures(
'parse/valid',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"compilerOptions": {
"allowJs": true,
"composite": true,
"strictNullChecks": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"files": [],
"include": [],
"references": [
{
"path": "./jsconfig.src.json"
},
{
"path": "./jsconfig.test.json"
}
],
"compilerOptions": {
"allowJs": true,
"maxNodeModuleJsDepth": 2,
"allowSyntheticDefaultImports": true,
"skipLibCheck": true,
"noEmit": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"extends": "./jsconfig.base",
"include": [
"src/**/*"
],
"exclude": [
"src/**/*.spec.js"
],
"compilerOptions": {
"strict": true,
"allowJs": true,
"composite": true,
"strictNullChecks": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"extends": "./jsconfig.base",
"include": [
"src/**/*.spec.js"
],
"compilerOptions": {
"strict": false,
"allowJs": true,
"composite": true,
"strictNullChecks": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"compilerOptions": {
"composite": true,
"strictNullChecks": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"extends": "./tsconfig.base",
"include": [
"src/**/*"
],
"exclude": [
"src/**/*.spec.ts"
],
"compilerOptions": {
"strict": true,
"composite": true,
"strictNullChecks": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"extends": "./tsconfig.base",
"include": [
"src/**/*.spec.ts"
],
"compilerOptions": {
"strict": false,
"composite": true,
"strictNullChecks": true
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"references": [
{
"path": "./src/tsconfig.src.json"
},
{
"path": "./tests/tsconfig.test.json"
}
],
"files": [],
"include": []
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"references": [
{
"path": "./src/tsconfig.src.json"
},
{
"path": "./tests/tsconfig.test.json"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"references": [
{
"path": "./src/tsconfig.src.json"
},
{
"path": "./tests/tsconfig.test.json"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"include": [
"./foo.ts"
],
"extends": "../tsconfig",
"compilerOptions": {
"composite": true,
"strict": true
}
}
Loading