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(generator-helper): continuously handle generator errors #19385

Merged
merged 27 commits into from May 26, 2023

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented May 22, 2023

  • Remove initWaitTime, handle errors properly all the time
  • Don't hang when a generator fails
  • Fix write EPIPE error when the generator stopped but the CLI still tries to send messages
  • Improve type safety

Fixes #3294
Fixes #8314
Fixes #10291
Fixes #14002

aqrln added 3 commits May 22, 2023 20:44
Handle generator process errors during the lifetime of the child
process and not just during the first 200 ms. This change completely
removes `initWaitTime`, which means that `prisma generate` command
will now be faster.
@aqrln aqrln added this to the 4.15.0 milestone May 23, 2023
packages/cli/src/bin.ts Outdated Show resolved Hide resolved
@codspeed-hq
Copy link

codspeed-hq bot commented May 24, 2023

CodSpeed Performance Report

Merging #19385 integration/generator-error-handling (3270c62) will degrade performances by 21.9%.

Congrats! CodSpeed is installed 🎉

🆕 0 new benchmarks were detected.

You will start to see performance impacts in the reports once the benchmarks are run from your default branch.

Detected benchmarks

  • getPlatform (70.4 ms)
  • client generation ~50 Models (9.4 s)
  • typescript compilation ~50 Models (188 ms)
  • client generation 100 models with relations (55 s)

@aqrln aqrln marked this pull request as ready for review May 24, 2023 12:21
@aqrln aqrln requested a review from a team as a code owner May 24, 2023 12:21
@aqrln aqrln requested review from millsp and Druue and removed request for a team May 24, 2023 12:21
packages/cli/src/bin.ts Outdated Show resolved Hide resolved
packages/cli/src/bin.ts Outdated Show resolved Hide resolved
packages/cli/src/bin.ts Outdated Show resolved Hide resolved
packages/cli/src/bin.ts Outdated Show resolved Hide resolved
@aqrln aqrln requested a review from Jolg42 May 25, 2023 14:01
Copy link
Member

@millsp millsp left a comment

Choose a reason for hiding this comment

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

The logic looks sound and better than it was, thank you!

@aqrln aqrln merged commit aa527d1 into main May 26, 2023
73 checks passed
@aqrln aqrln deleted the integration/generator-error-handling branch May 26, 2023 14:50
aqrln added a commit that referenced this pull request Jun 16, 2023
Unless `wait: true` is passed to `open`, it does not install an `error`
event listener to the child process, so it's on us to handle it.

Before #19385 all errors that
happen in CLI were silenly ignored, so this issue was hidden. This PR
fixes it.

The added e2e test fails with `Error: spawn xdg-open ENOENT` without
the changes in `Studio.ts`, and passes with them.

Adding special debug logs that the test depends on was necessary since
the error happens asynchronously later, so we can't depend on the
"Prisma Studio is up on http://localhost:5555" message, and we need to
depend on some output that's printed after the point where the ENOENT
error may have happened to determine we can send SIGINT to the child
process so that test doesn't hang infinitely.

An alternative would've been to rely on a timeout to terminate the
process, but that would be unreliable/flaky.

Fixes: prisma/studio#1128
aqrln added a commit that referenced this pull request Jun 16, 2023
…19812)

Unless `wait: true` is passed to `open`, it does not install an `error`
event listener to the child process, so it's on us to handle it.

Before #19385 all errors that
happen in CLI were silenly ignored, so this issue was hidden. This PR
fixes it.

The added e2e test fails with `Error: spawn xdg-open ENOENT` without
the changes in `Studio.ts`, and passes with them.

Adding special debug logs that the test depends on was necessary since
the error happens asynchronously later, so we can't depend on the
"Prisma Studio is up on http://localhost:5555" message, and we need to
depend on some output that's printed after the point where the ENOENT
error may have happened to determine we can send SIGINT to the child
process so that test doesn't hang infinitely.

An alternative would've been to rely on a timeout to terminate the
process, but that would be unreliable/flaky.

Fixes: prisma/studio#1128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants