-
-
Notifications
You must be signed in to change notification settings - Fork 738
fix: don't use sync fs api #12353
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: don't use sync fs api #12353
Conversation
✅ Deploy Preview for rspack canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR converts synchronous filesystem operations to async in the context module resolution pipeline, preventing blocking operations in the tokio runtime. The changes ensure filesystem calls (metadata, read_dir) are properly awaited rather than using their synchronous variants, which aligns with the PR title's goal to "not sync fs api in tokio thread".
Key changes:
- Converted
visit_dirsfunction to async with recursive directory traversal - Changed
ResolveContextModuleDependenciestype signature from synchronous to async (returningBoxFuture) - Updated all
resolve_dependenciesclosures to returnBox::pin(async move { ... })for async execution
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/rspack_core/src/context_module_factory.rs | Converted visit_dirs to async with #[async_recursion], changed fs calls from metadata_sync/read_dir_sync to their async equivalents, and wrapped resolve_dependencies closure in async block |
| crates/rspack_core/src/context_module.rs | Updated ResolveContextModuleDependencies type to return BoxFuture and added .await to resolve_dependencies call in build method |
| crates/rspack_plugin_module_replacement/src/context_replacement.rs | Wrapped resolve_dependencies closure in Box::pin(async move { ... }) to match new async signature, added Arc wrapping for captured HashMap |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
crates/rspack_plugin_module_replacement/src/context_replacement.rs
Outdated
Show resolved
Hide resolved
Rsdoctor Bundle Diff AnalysisFound 5 project(s) in monorepo. 📁 react-1kPath:
📦 Download Diff Report: react-1k Bundle Diff 📁 react-5kPath:
📦 Download Diff Report: react-5k Bundle Diff 📁 romePath:
📦 Download Diff Report: rome Bundle Diff 📁 react-10kPath:
📦 Download Diff Report: react-10k Bundle Diff 📁 ui-componentsPath:
📦 Download Diff Report: ui-components Bundle Diff Generated by Rsdoctor GitHub Action |
📦 Binary Size-limit
❌ Size increased by 2.50KB from 47.70MB to 47.71MB (⬆️0.01%) |
…t.rs Co-authored-by: Copilot <[email protected]>
CodSpeed Performance ReportMerging #12353 will not alter performanceComparing Summary
|
Summary
close #12342
In ContextModule related plugins, filesystem sync APIs are called which is implemented by wrapping async fs API in
block_on. And Rspack is running under ablock_onalready. This will trigger a panicSolution
Using async api.
And sync fs Api should be forbidden later then?
Related links
Checklist