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(gatsby): try to automatically recover when parcel segfaults #38773

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Dec 20, 2023

Description

Seems like ARM macs parcel (at least version we use) cause quite a bit of problems and results in often seg faults - this tries to run parcel in child process so we can try to handle segfaults by clearing just parcel cache and retrying (few times)

Documentation

Tests

Related Issues

Possibly fixes #38483

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 20, 2023
Comment on lines 121 to 128
let cacheClosePromise = Promise.resolve()
let bundles: RunParcelReturn
try {
// @ts-ignore store is public field on LMDBCache class, but public interface for Cache
// doesn't have it. There doesn't seem to be proper public API for this, so we have to
// resort to reaching into internals. Just in case this is wrapped in try/catch if
// parcel changes internals in future (closing cache is only needed when retrying
// so the if the change happens we shouldn't fail on happy builds)
cacheClosePromise = cache.store.close()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are killing child process so this should be done automatically on process exits

@pieh pieh added topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 20, 2023
Comment on lines +13 to +34
jest.mock(`gatsby-worker`, () => {
const gatsbyWorker = jest.requireActual(`gatsby-worker`)

const { WorkerPool: OriginalWorkerPool } = gatsbyWorker

class WorkerPoolThatCanUseTS extends OriginalWorkerPool {
constructor(workerPath, options) {
options.env = {
...(options.env ?? {}),
NODE_OPTIONS: `--require ${require.resolve(
`./packages/gatsby/src/utils/worker/__tests__/test-helpers/ts-register.js`
)}`,
}
super(workerPath, options)
}
}

return {
...gatsbyWorker,
WorkerPool: WorkerPoolThatCanUseTS,
}
})
Copy link
Contributor Author

@pieh pieh Dec 20, 2023

Choose a reason for hiding this comment

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

This is just a trick for unit tests to be able to use source TS (and ESM) files when using gatsby-worker / child process setup

In actual usage TS source files become CJS and those will be used (all e2e tests would be using that for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added in this PR and as global mock because compile-gatsby-files is being directly and indrectly used in quite a bit of unit tests, so to avoid hunting for all indirect usage we just allow that to happen any time gatsby-worker is used within those tests

@pieh pieh changed the title fix(gatsby): try to automatically recover when parcel segfauls fix(gatsby): try to automatically recover when parcel segfaults Dec 20, 2023
@pieh pieh marked this pull request as ready for review December 21, 2023 06:24
@pieh pieh force-pushed the fix/try-to-self-heal-from-parcel-seg-fault branch from 2ac6db2 to 3e72735 Compare December 21, 2023 08:31
Comment on lines +191 to +195
"filePath": "<PROJECT_ROOT>/gatsby-node.ts",
"generalMessage": "Expected ';', '}' or <eof>",
"hints": null,
"origin": "@parcel/transformer-js",
"specificMessage": "This is the expression part of an expression statement",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is improvements to be made here to point out specific location as error we get is like this:

{
      diagnostics: [
        {
          message: "Expected ';', '}' or <eof>",
          codeFrames: [
            {
              filePath: '/Users/misiek/dev/pgs-gatsby/packages/gatsby/src/utils/parcel/__tests__/fixtures/error-in-code-ts/gatsby-node.ts',
              codeHighlights: [
                {
                  message: 'This is the expression part of an expression statement',
                  start: { line: 5, column: 1 },
                  end: { line: 5, column: 4 }
                },
                {
                  message: null,
                  start: { line: 5, column: 6 },
                  end: { line: 5, column: 7 }
                }
              ]
            }
          ],
          hints: null,
          origin: '@parcel/transformer-js'
        }
      ]
    }

but it's out of scope of this PR - purpose here is to try to handle segfaults without introducing regression - this added tests is just just a test for case we didn't have (what's the behavior on ts files that are invalid) and that I did break before my most recent commit

@pieh pieh force-pushed the fix/try-to-self-heal-from-parcel-seg-fault branch from 3e72735 to 6360181 Compare December 21, 2023 08:47
@laurogripa
Copy link

Nice to know someone is working on this issue!

I'm trying to run your branch as a local version of Gatsby in my project because this issue makes it almost impossible for me to develop.
I added "gatsby": "file:../gatsby" to my package.json (the relative path is correct), the installation went okay but I'm getting this errors:

➜  erc55 git:(main) ✗ npm run develop
> [email protected] develop
> gatsby develop
sh: gatsby: command not found

➜  erc55 git:(main) ✗ npx gatsby develop
npm ERR! Cannot read properties of undefined (reading 'replace')

Any suggestions on how to run your branch locally or should I just wait for it to get merged and published?

@pieh
Copy link
Contributor Author

pieh commented Jan 2, 2024

@laurogripa I published canary version with those changes [email protected] if you want to try it out

import { working } from "../utils/say-what-ts"
import { createPages } from "../utils/create-pages-ts"

this is wrong syntax that should't compile
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this line be a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh or is this there to fail the compilation 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's test fixture to test behavior when there is error in source file that should result in compilation problem and test itself is here

it(`handles errors in TS code`, async () => {
process.chdir(dir.errorInCode)
await remove(`${dir.errorInCode}/.cache`)
await compileGatsbyFiles(dir.errorInCode)
expect(reporterPanicMock).toMatchInlineSnapshot(`
[MockFunction] {
"calls": Array [
Array [
Object {
"context": Object {
"filePath": "<PROJECT_ROOT>/gatsby-node.ts",
"generalMessage": "Expected ';', '}' or <eof>",
"hints": null,
"origin": "@parcel/transformer-js",
"specificMessage": "This is the expression part of an expression statement",
},
"id": "11901",
},
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
],
}
`)
})
})

I did break this behavior when iterating on this code and noticed we didn't have tests for this case so I added it as part of this PR

#38773 (comment) this comment highlight opportunity to improve error message possibly, but this was just out of scope of the fix

@pieh pieh merged commit 0a80cd6 into master Jan 9, 2024
33 checks passed
@pieh pieh deleted the fix/try-to-self-heal-from-parcel-seg-fault branch January 9, 2024 08:55
pieh added a commit that referenced this pull request Jan 9, 2024
* fix(gatsby): try to automatically recover when parcel segfauls

* test: make gatsby-worker test adjustment global

* fix: handle actual compilation errors

* test: bump timeout for windows

* init bundles array so TS is happy

(cherry picked from commit 0a80cd6)
pieh added a commit that referenced this pull request Jan 10, 2024
…) (#38799)

* fix(gatsby): try to automatically recover when parcel segfauls

* test: make gatsby-worker test adjustment global

* fix: handle actual compilation errors

* test: bump timeout for windows

* init bundles array so TS is happy

(cherry picked from commit 0a80cd6)

Co-authored-by: Michal Piechowiak <[email protected]>
This was referenced May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: TypeScript Issues and PRs related to TS in general, public typings or gatsby-plugin-typescript
Projects
Status: Published
Development

Successfully merging this pull request may close these issues.

Random "SIGSEGV" Errors in "yarn build" Following Failed/Canceled Build
3 participants