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(typecheck-worker): avoid false-positive willTypechecks on Linux #1198

Merged
merged 1 commit into from Jul 3, 2020

Conversation

dfreeman
Copy link
Member

@dfreeman dfreeman commented Jul 3, 2020

We've seen occasional timeouts in CI for our test that verifies we don't block builds for file changes that don't trigger a new typecheck. Interestingly, these timeouts have never occurred on Windows, and I've also never managed to reproduce them locally on my Mac, but they pretty reliably occur every 3-4 runs on Linux.

Digging in (by running the offending test in a loop in CI), it seems that the change to a .hbs file is occasionally causing TypeScript's createProgram to be called, but then subsequently short circuited as some other piece of machinery discovers that the change in question doesn't matter. This is a problem for us, as we treat createProgram as proof that a typecheck has started, and if afterProgramCreate is never subsequently called, then our worker just sits on a pending promise forever.

Fortunately for us, when afterProgramCreate is called, it's called synchronously near the end of createProgram, so from the perspective of the outside world, it doesn't matter if we call willTypecheck in createProgram or afterProgramCreate: no events will be dispatched in between those two functions anyway.

@dfreeman
Copy link
Member Author

dfreeman commented Jul 3, 2020

P.S. I ran the offending test 50 times in a loop in CI and didn't get any more hangs with this change.

Copy link
Member

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

I can't say I love this, but I do love that you fixed it!

@dfreeman dfreeman merged commit 8162cb1 into master Jul 3, 2020
@dfreeman dfreeman deleted the test-race-condition branch July 3, 2020 15:21
@chriskrycho
Copy link
Member

For clarification on my comment for any lookers-on/posterity: while I would guess this is fairly stable behavior, I inherently dislike solutions that leave us at the mercy of changes to an API that could change whenever convenient for the TS team—but since it's fairly core to the compiler API, it seems reasonable and we expect that it would make release notes/etc. if it were to change. And in truth, we're always in that position so…

@jamescdavis
Copy link
Member

Thank you, @dfreeman !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants