Skip to content

perf(fs): don't canonicalize path when opening file if --allow-all is passed #28716

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

nathanwhit
Copy link
Member

Fixes #28702.

@nathanwhit nathanwhit requested a review from Copilot April 3, 2025 00:35
@nathanwhit nathanwhit changed the title perf(fs): don't canonicalize path when opening path if --allow-all is passed perf(fs): don't canonicalize path when opening file if --allow-all is passed Apr 3, 2025
Copy link

@Copilot Copilot AI left a 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 improves performance in filesystem operations by avoiding unnecessary canonicalization when the "--allow-all" flag is passed. Key changes include:

  • Adding a new public method "allows_all" in the permissions container to check for full permission grants.
  • Updating synchronous and asynchronous permission check functions in ext/fs/ops.rs to skip additional checks when all permissions are granted.
  • Extending the FsPermissions trait in ext/fs/lib.rs to include an "allows_all" method.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
runtime/permissions/lib.rs Adds the "allows_all" method to check if full permissions are granted
ext/fs/ops.rs Updates permission check functions to conditionally bypass checks
ext/fs/lib.rs Extends the FsPermissions trait with an "allows_all" method implementation

ext/fs/ops.rs Outdated
permissions.allows_all()
};
if allows_all {
None
Copy link
Member

Choose a reason for hiding this comment

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

How hard is it to move the canonicalization stuff into FsPermissions::check_open? Ideally we'd only be accessing Permissions once from the OpState and then aquiring its lock once instead of now twice when not running with -A.

@nathanwhit nathanwhit force-pushed the allow-all-skip-realpath branch from 82a961a to bc6ecc1 Compare April 11, 2025 01:18
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.

Deno.open performs multiple syscalls even if --allow-all is passed
2 participants