这是indexloc提供的服务,不要输入任何密码
Skip to content

fix(watch): await persistent run task instead of abort #9152

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

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

chris-olszewski
Copy link
Member

Description

This PR fixes an issue where the TUI was getting events from a previous run after an update_tasks call causing it to exit.

One can tell this is happening by inspecting the logs:

2024-09-16T13:17:16.509-0400 [DEBUG] turborepo_ui::tui::app: updating task list: ["docs#dev", "web#dev"]
...
2024-09-16T13:17:16.512-0400 [DEBUG] turborepo_ui::tui::app: finishing task web#dev

The TUI is receiving that web#dev finish event from the previous run which confuses the TUI as web#dev hasn't been started yet in this run. This appears to only happen for tasks that don't exit within a 500ms of receiving SIGINT and need to be forcibly killed.

Testing Instructions

Use create-turbo to create a basic repro. Change one of the dev scripts to have a long running SIGINT handler:

"dev": "trap 'sleep 5 && echo fin' SIGINT; next dev --turbo"

Observe that editing root package.json (just adding/changing "description" will do) while turbo watch dev is running results in a crash:
Screenshot 2024-09-16 at 1 36 03 PM

Checkout the changes in this PR and try the same thing. You should observe that turbo_dev --skip-infer watch dev no longer crashes on a root package.json change.
Screenshot 2024-09-16 at 1 37 25 PM
You can see that the SIGINT handler has not run as there is no fin displayed in the output on the restart.

Copy link

vercel bot commented Sep 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 16, 2024 5:40pm
8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm
examples-designsystem-docs ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm
examples-gatsby-web ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm
examples-native-web ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm
examples-svelte-web ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm
examples-tailwind-web ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm
examples-vite-web ⬜️ Ignored (Inspect) Sep 16, 2024 5:40pm

@chris-olszewski chris-olszewski marked this pull request as ready for review September 16, 2024 17:40
@chris-olszewski chris-olszewski requested a review from a team as a code owner September 16, 2024 17:40
run_task.abort();
// Run should exit shortly after we stop all child tasks, wait for it to finish
// to ensure all messages are flushed.
let _ = run_task.await;
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't abort as that seems to still gives us messages in the pipes. I think this is due to the TUI channels being from std::sync::mpsc instead of tokio::sync::mpsc (For now).

@chris-olszewski chris-olszewski merged commit c3b7be9 into main Sep 16, 2024
40 checks passed
@chris-olszewski chris-olszewski deleted the olszewski/fix_watch_tui_exit branch September 16, 2024 18:20
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.

2 participants