microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.69k stars 12.44k forks source link

4.8 hasSameBuildInfo can randomly crash tsc.js with "Cannot read properties of undefined (reading 'path')" #50545

Closed dylang closed 2 years ago

dylang commented 2 years ago

Bug Report

Multiple packages in the monorepo are being built simultaneously when this happens:

// This is where we are building each package to be published.
const results = await pMap(packagePaths, async (packagePath) => buildForNpm({ packagePath, groupName }), {
  // Change to 1 and the problem seems to go away, 
  // but it will take significantly longer to build.
  concurrency: 20 
});

buildForNpm runs tsc --build tsconfig.json --pretty

        if (refBuildInfo.path === buildInfoCacheEntry.path)
                         ^

TypeError: Cannot read properties of undefined (reading 'path')
    at hasSameBuildInfo (/project/node_modules/typescript/lib/tsc.js:105509:26)
    at hasSameBuildInfo (/project/node_modules/typescript/lib/tsc.js:105517:21)
    at getUpToDateStatusWorker (/project/node_modules/typescript/lib/tsc.js:105453:44)
    at getUpToDateStatus (/project/node_modules/typescript/lib/tsc.js:105531:22)
    at getNextInvalidatedProjectCreateInfo (/project/node_modules/typescript/lib/tsc.js:105082:26)
    at getNextInvalidatedProject (/project/node_modules/typescript/lib/tsc.js:105142:20)
    at build (/project/node_modules/typescript/lib/tsc.js:105665:38)
    at Object.build (/project/node_modules/typescript/lib/tsc.js:105863:101)
    at performBuild (/project/node_modules/typescript/lib/tsc.js:106569:73)
    at Object.executeCommandLine (/project/node_modules/typescript/lib/tsc.js:106514:24)

Once the problem happens, the problem will keep happening until the output in cache/outDir is deleted.

The error is thrown here: https://github.com/microsoft/TypeScript/blob/main/src/compiler/tsbuildPublic.ts#L1794

Possibly related to the changes in this PR: https://github.com/microsoft/TypeScript/pull/48784.

This fixes the problem for me. Happy to make a PR if I can figure out how to test it.

-if (refBuildInfo.path === buildInfoCacheEntry.path) return true;
+if (refBuildInfo?.path === buildInfoCacheEntry.path) return true;

πŸ”Ž Search Terms

πŸ•— Version & Regression Information

⏯ Playground Link

Not able to reproduce with small projects.

πŸ’» Code

This is happening in a large closed-source project.

πŸ™ Actual behavior

Crashing during build.

πŸ™‚ Expected behavior

No crash.

DanielRosenwasser commented 2 years ago

Are each of these projects using the same outDir with the same .tsbuildinfo output path?

sheetalkamat commented 2 years ago

To me it seems like concurrent building is causing this which tsbuild is not equipped to handle. That is when project A is being built or checked for what to build all its references should be already built or have errors. So thats why when someone calls this method it is expected to find the buildinfo.

dylang commented 2 years ago

Hi @DanielRosenwasser and @sheetalkamat - thank you for taking a look at this issue!

Are each of these projects using the same outDir with the same .tsbuildinfo output path?

No, they all have unique paths.

To me it seems like concurrent building is causing this which tsbuild is not equipped to handle.

We've been using this for years without issue, so I think tsbuild handles this use case well otherwise.

In the code:

const refBuildInfo = state.buildInfoCache.get(resolvedRefPath)!;

I think that ! at the end means state.buildInfoCache.get will always return a value, right? That doesn't seem like a safe assumption with any cache, so maybe this is a bug?

sheetalkamat commented 2 years ago

This can return undefined in general cache but because build is assumed to have already built references of the project it should have buildinfo cached. So the assumption is correct. Relying on this assumption speeds up the project build and we dont need to do lots of timestamp checks so buildInfo for referenced project should be present i think. This is part of speedup work we did by caching build info and relying only on buildInfo to give us correct information about whether to rebuild or not.

Eg we also use dts file change time to detemine what and when to build so presence of buildInfo of referenced project gives us this speed up and we assume this in multiple places especially #48784 relies on this fact and sees huge perf improvement.

DanielRosenwasser commented 2 years ago

@sheetalkamat I'm not an expert here, so what exactly is happening that invalidates the assumption? And what would happen if we made the function defensive in case of a non-existent build info?

sheetalkamat commented 2 years ago

I mean we can make this defensive but i am more worried about why is it running into this case. For all referenced projects we should have buildInfo cached as part of their upto date checking or the referenced project is unbuildable and that is already handled.

Are we missing something? Are there other places where this assumption is wrong. Thats why i dont think just writing defensive code there is right thing to do. I would like to get to bottom of this issue and if result of that is just making this code path more defensive that's fine but we should analyze since this is basic assumption under which build mode works and we might have missed something.

@dylang can you please create a small repro so i can look into this and then analyse whats going on based on that. If creating repro is difficult logs from running verbose mode should be helpful too.

Thanks

dylang commented 2 years ago

Hi @sheetalkamat, I appreciate your interest in figuring this out, thank you!

I'm not able to reproduce this in a small repo.

Do you have one with many packages and uses references that you use for performance measuring? I can start there.

sheetalkamat commented 2 years ago

Can you share logs from running tsc -b -verbose log.. or your repro even though not small ..

dylang commented 2 years ago

@sheetalkamat I just figured something by logging resolvedRefPath from var refBuildInfo = state.buildInfoCache.get(resolvedRefPath);!

It's always erroring with the same package, so it's not a race condition. It's something with that package in our monorepo...

And good news: That package is a tiny one, a single JS file! Hmm, no typescript. πŸ˜„πŸ’‘

When I add a single typescript file to that package the problem goes away.

So it still seems like a bug in tsc, but at least I have a better workaround than disabling concurrency.

dylang commented 2 years ago

Here's the verbose output from one of the packages that fails.

/projects/dylang-prs/config/browserslist-config-dylang/tsconfig.json is the package with one JS file and no typescript.

[12:50:07 PM] Projects in this build:
    * ../../test-helpers/dylang-mocker/tsconfig.json
    * ../../config/dylang-config/tsconfig.json
    * ../../helpers/dylang-logger/tsconfig.json
    * ../../helpers/dylang-tools/tsconfig.json
    * ../../config/dylang-oa-config/tsconfig.json
    * ../../config/dylang-typescript/tsconfig.json
    * ../../helpers/dylang-github/tsconfig.json
    * ../../config/browserslist-config-dylang/tsconfig.json
    * ../../config/babel-preset-dylang/tsconfig.json
    * ../../config/dylang-webpack/tsconfig.json
    * ../dylang-build/tsconfig.json
    * ../../helpers/dylang-cdn-storage/tsconfig.json
    * ../../helpers/dylang-changelog/tsconfig.json
    * ../dylang-publish/tsconfig.json
    * tsconfig.json

[12:50:07 PM] Project '../../test-helpers/dylang-mocker/tsconfig.json' is up to date because newest input '../../test-helpers/dylang-mocker/src/mocked.ts' is older than output '../../.cache/ts/test-helpers/dylang-mocker/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../config/dylang-config/tsconfig.json' is up to date because newest input '../../config/dylang-config/src/get-config-from-source/generate-config-error.ts' is older than output '../../.cache/ts/config/dylang-config/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../helpers/dylang-logger/tsconfig.json' is up to date because newest input '../../helpers/dylang-logger/src/log-storage/create-log-writer.ts' is older than output '../../.cache/ts/helpers/dylang-logger/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../helpers/dylang-tools/tsconfig.json' is up to date because newest input '../../helpers/dylang-tools/src/print-big-text.test.ts' is older than output '../../.cache/ts/helpers/dylang-tools/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../config/dylang-oa-config/tsconfig.json' is up to date because newest input '../../config/dylang-oa-config/src/get-module-config.ts' is older than output '../../.cache/ts/config/dylang-oa-config/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../config/dylang-typescript/tsconfig.json' is up to date because newest input '../../config/dylang-typescript/src/index.ts' is older than output '../../.cache/ts/config/dylang-typescript/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../helpers/dylang-github/tsconfig.json' is up to date because newest input '../../helpers/dylang-github/src/git/git.test.ts' is older than output '../../.cache/ts/helpers/dylang-github/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../config/babel-preset-dylang/tsconfig.json' is up to date because newest input '../../config/babel-preset-dylang/src/babel.config.test.js' is older than output '../../.cache/ts/config/babel-preset-dylang/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../../config/dylang-webpack/tsconfig.json' is up to date because newest input '../../config/dylang-webpack/src/webpack.config.ts' is older than output '../../.cache/ts/config/dylang-webpack/tsconfig.tsbuildinfo'

[12:50:07 PM] Project '../dylang-build/tsconfig.json' is out of date because output '../../.cache/ts/build-tools/dylang-build/tsconfig.tsbuildinfo' is older than input '../dylang-build/src/compile-code/run-tsc.ts'

[12:50:07 PM] Building project '/projects/dylang-prs/build-tools/dylang-build/tsconfig.json'...

[12:50:11 PM] Project '../../helpers/dylang-cdn-storage/tsconfig.json' is up to date because newest input '../../helpers/dylang-cdn-storage/src/ModernizedS3Api.ts' is older than output '../../.cache/ts/helpers/dylang-cdn-storage/tsconfig.tsbuildinfo'

[12:50:11 PM] Project '../../helpers/dylang-changelog/tsconfig.json' is up to date because newest input '../../helpers/dylang-changelog/src/create-github-release.test.ts' is older than output '../../.cache/ts/helpers/dylang-changelog/tsconfig.tsbuildinfo'

/projects/dylang-prs/config/browserslist-config-dylang/tsconfig.json
/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105512
        if (refBuildInfo.path === buildInfoCacheEntry.path)
                         ^

TypeError: Cannot read properties of undefined (reading 'path')
    at hasSameBuildInfo (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105512:26)
    at hasSameBuildInfo (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105520:21)
    at hasSameBuildInfo (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105520:21)
    at getUpToDateStatusWorker (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105453:44)
    at getUpToDateStatus (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105534:22)
    at getNextInvalidatedProjectCreateInfo (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105082:26)
    at getNextInvalidatedProject (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105142:20)
    at build (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105668:38)
    at Object.build (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:105866:101)
    at performBuild (/projects/dylang-prs/node_modules/typescript/lib/tsc.js:106572:73)
sheetalkamat commented 2 years ago

Can you please post tsconfig for ../../helpers/dylang-changelog/tsconfig.json and all the references recursively.. Is there solution style reference there in somewhere.. just guessing..

Edit: dont think there is any solution by looking at the verbose log.

Also if possible can you let me know what is the resolvedRefPath when the hasSameBuildInfo is failing to find build info. Because from the log it seems the buildInfos should have been cached for all projects up until that point.

sheetalkamat commented 2 years ago

Also [12:50:07 PM] Building project '/projects/dylang-prs/build-tools/dylang-build/tsconfig.json'... is this the action that happens in parallel.. and is this the one its failing on?

sheetalkamat commented 2 years ago

And good news: That package is a tiny one, a single JS file! Hmm, no typescript. πŸ˜„πŸ’‘

Does this have noEmit ?

Is this referenced by project indirectly or directly that is failing.

dylang commented 2 years ago

resolvedRefPath when the hasSameBuildInfo is failing to find build info

resolvedRefPath is /projects/dylang-prs/config/browserslist-config-dylang/tsconfig.json - that's the package with no typescript.

Also [12:50:07 PM] Building project '/projects/dylang-prs/build-tools/dylang-build/tsconfig.json'... is this the action that happens in parallel.. and is this the one its failing on?

This is one of many that are building in parallel. It is one of 4 that is failing. All four fail with the same resolvedRefPath as above.

Does this have noEmit ?

I don't think that's possible with --build.

Is this referenced by project indirectly or directly that is failing.

Indirectly. It is referenced by our babel and webpack packages, but those packages aren't failing.

@m/dylang-typescript - Everything extends this one ```json { "$schema": "https://json.schemastore.org/tsconfig", "// Note //": "This file is automatically maintained by dylang.", "include": [ "src/**/*.ts", "src/**/*.tsx" ], "compilerOptions": { "composite": true, "declaration": true, "downlevelIteration": true, "emitDecoratorMetadata": true, "esModuleInterop": true, "experimentalDecorators": true, "forceConsistentCasingInFileNames": true, "jsx": "react", "lib": [ "ES2019", "ES2020.BigInt", "ES2020.Promise", "ES2020.String", "ES2020.Intl", "ES2021.String", "ESNext.String", "DOM", "DOM.Iterable" ], "moduleResolution": "node", "noFallthroughCasesInSwitch": true, "noImplicitAny": true, "noImplicitReturns": true, "noImplicitThis": true, "noUnusedLocals": true, "outDir": "../../.cache/ts/config/dylang-typescript", "resolveJsonModule": true, "rootDir": ".", "sourceMap": true, "strict": true, "strictNullChecks": true, "suppressImplicitAnyIndexErrors": false, "tsBuildInfoFile": "../../.cache/ts/config/dylang-typescript/tsconfig.tsbuildinfo", "types": [ "node", "jest" ], "useUnknownInCatchVariables": false, "module": "commonjs", "noEmit": false, "skipLibCheck": true, "target": "ES2019" }, "references": [] } ```
../../helpers/dylang-changelog/tsconfig.json ```json { "$schema": "https://json.schemastore.org/tsconfig", "// Note //": "This file is automatically maintained by dylang.", "extends": "@m/dylang-typescript", "include": [ "./src/**/*" ], "compilerOptions": { "rootDir": ".", "outDir": "../../.cache/ts/helpers/dylang-changelog", "composite": true, "tsBuildInfoFile": "../../.cache/ts/helpers/dylang-changelog/tsconfig.tsbuildinfo" }, "references": [ { "path": "../../config/dylang-config/tsconfig.json" }, { "path": "../dylang-github/tsconfig.json" }, { "path": "../dylang-tools/tsconfig.json" }, { "path": "../../test-helpers/dylang-mocker/tsconfig.json" } ] } ```

To retrieve all the references by hand will take me too much time. There are over 100 packages in this repo. If you have a tool or script I'd be happy to run it, or happy to jump on video chat.

sheetalkamat commented 2 years ago

Thanks for helping out with info on this one. I will try to create repro with this info but in mean time can you get me contents of /projects/dylang-prs/config/browserslist-config-dylang/tsconfig.json

it feels like this is project with no files and thats what is causing issue. (I missed this one in your log.. i see now that it didnt show as uptodate or not in the --v log because it is a project with zero files and just references ?)

dylang commented 2 years ago

Here is /projects/dylang-prs/config/browserslist-config-dylang/tsconfig.json:

It does not have any dependencies and therefor no references.

{
  "$schema": "https://json.schemastore.org/tsconfig",
  "// Note //": "This file is automatically maintained by dylang.",
  "extends": "@m/dylang-typescript",
  "include": [
    "./src/**/*"
  ],
  "compilerOptions": {
    "rootDir": ".",
    "outDir": "../../.cache/ts/config/browserslist-config-magic",
    "composite": true,
    "tsBuildInfoFile": "../../.cache/ts/config/browserslist-config-magic/tsconfig.tsbuildinfo"
  },
  "references": []
}
sheetalkamat commented 2 years ago

@dylang thanks for all the info. i am working on the fix. But just so you know /projects/dylang-prs/config/browserslist-config-dylang/tsconfig.json isnt building anything.. you mentioned something about javascript file being there but since you dont have allowJs or checkJs it isnt included in the project and because you have "references: []" it is treated as solution config and you arent getting any error about no files found per configuration.

dylang commented 2 years ago

Thanks @sheetalkamat. Adding allowJs did indeed correct the problem on my side. Thanks, also, for acknowledging this as a bug.