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

perf(node/fs): faster existsSync when not exists #21458

Merged
merged 7 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion cli/tests/unit_node/fs_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import { assert, assertThrows } from "../../../test_util/std/assert/mod.ts";
import { join } from "node:path";
import { tmpdir } from "node:os";
import { mkdtempSync, readFileSync, writeFileSync } from "node:fs";
import { existsSync, mkdtempSync, readFileSync, writeFileSync } from "node:fs";
import { pathToAbsoluteFileUrl } from "../unit/test_util.ts";

Deno.test(
"[node/fs writeFileSync] write file without option",
Expand Down Expand Up @@ -43,3 +44,39 @@ Deno.test(
);
},
);

Deno.test(
"[node/fs existsSync] path",
{ permissions: { read: true } },
() => {
assert(existsSync("cli/tests/testdata/assets/fixture.json"));
},
);

Deno.test(
"[node/fs existsSync] url",
{ permissions: { read: true } },
() => {
assert(existsSync(
pathToAbsoluteFileUrl("cli/tests/testdata/assets/fixture.json"),
));
},
);

Deno.test(
"[node/fs existsSync] no permission",
{ permissions: { read: false } },
() => {
assertThrows(() => {
existsSync("cli/tests/testdata/assets/fixture.json");
}, Deno.errors.PermissionDenied);
},
);

Deno.test(
"[node/fs existsSync] not exists",
{ permissions: { read: true } },
() => {
assert(!existsSync("bad_filename"));
},
);
16 changes: 14 additions & 2 deletions ext/node/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,14 @@ pub trait NodePermissions {
url: &Url,
api_name: &str,
) -> Result<(), AnyError>;
fn check_read(&self, path: &Path) -> Result<(), AnyError>;
fn check_read(&self, path: &Path) -> Result<(), AnyError> {
self.check_read_with_api_name(path, None)
}
fn check_read_with_api_name(
&self,
path: &Path,
api_name: Option<&str>,
) -> Result<(), AnyError>;
fn check_sys(&self, kind: &str, api_name: &str) -> Result<(), AnyError>;
}

Expand All @@ -64,7 +71,11 @@ impl NodePermissions for AllowAllNodePermissions {
) -> Result<(), AnyError> {
Ok(())
}
fn check_read(&self, _path: &Path) -> Result<(), AnyError> {
fn check_read_with_api_name(
&self,
_path: &Path,
_api_name: Option<&str>,
) -> Result<(), AnyError> {
Ok(())
}
fn check_sys(&self, _kind: &str, _api_name: &str) -> Result<(), AnyError> {
Expand Down Expand Up @@ -227,6 +238,7 @@ deno_core::extension!(deno_node,
ops::crypto::x509::op_node_x509_get_valid_to,
ops::crypto::x509::op_node_x509_get_serial_number,
ops::crypto::x509::op_node_x509_key_usage,
ops::fs::op_node_fs_exists_sync<P>,
ops::winerror::op_node_sys_to_uv_error,
ops::v8::op_v8_cached_data_version_tag,
ops::v8::op_v8_get_heap_statistics,
Expand Down
26 changes: 26 additions & 0 deletions ext/node/ops/fs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.

use std::path::PathBuf;

use deno_core::error::AnyError;
use deno_core::op2;
use deno_core::OpState;
use deno_fs::FileSystemRc;

use crate::NodePermissions;

#[op2(fast)]
pub fn op_node_fs_exists_sync<P>(
state: &mut OpState,
#[string] path: String,
) -> Result<bool, AnyError>
where
P: NodePermissions + 'static,
{
let path = PathBuf::from(path);
state
.borrow_mut::<P>()
.check_read_with_api_name(&path, Some("node:fs.existsSync()"))?;
let fs = state.borrow::<FileSystemRc>();
Ok(fs.lstat_sync(&path).is_ok())
}
1 change: 1 addition & 0 deletions ext/node/ops/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.

pub mod crypto;
pub mod fs;
pub mod http;
pub mod http2;
pub mod idna;
Expand Down
9 changes: 2 additions & 7 deletions ext/node/polyfills/_fs/_fs_exists.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

// TODO(petamoriken): enable prefer-primordials for node polyfills
// deno-lint-ignore-file prefer-primordials

const core = globalThis.__bootstrap.core;
import { pathFromURL } from "ext:deno_web/00_infra.js";

type ExistsCallback = (exists: boolean) => void;
Expand Down Expand Up @@ -35,10 +35,5 @@ Object.defineProperty(exists, kCustomPromisifiedSymbol, {
*/
export function existsSync(path: string | URL): boolean {
path = path instanceof URL ? pathFromURL(path) : path;
try {
Deno.lstatSync(path);
return true;
} catch (_err) {
return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

One other thing this fixes is previously this would supress permission errors.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC this was done on purpose, but seems fishy. @kt3k any idea here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If intentional, my guess would be to reduce permission prompts, but I don't think we should do that because it causes subtle bugs.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it was for avoiding to throw 'Not a directory' error when called with non-existing dir denoland/std#2494

}
return core.ops.op_node_fs_exists_sync(path);
}
8 changes: 6 additions & 2 deletions runtime/permissions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1376,8 +1376,12 @@ impl deno_node::NodePermissions for PermissionsContainer {
}

#[inline(always)]
fn check_read(&self, path: &Path) -> Result<(), AnyError> {
self.0.lock().read.check(path, None)
fn check_read_with_api_name(
&self,
path: &Path,
api_name: Option<&str>,
) -> Result<(), AnyError> {
self.0.lock().read.check(path, api_name)
}

fn check_sys(&self, kind: &str, api_name: &str) -> Result<(), AnyError> {
Expand Down
6 changes: 5 additions & 1 deletion runtime/snapshot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,11 @@ impl deno_node::NodePermissions for Permissions {
) -> Result<(), deno_core::error::AnyError> {
unreachable!("snapshotting!")
}
fn check_read(&self, _p: &Path) -> Result<(), deno_core::error::AnyError> {
fn check_read_with_api_name(
&self,
_p: &Path,
_api_name: Option<&str>,
) -> Result<(), deno_core::error::AnyError> {
unreachable!("snapshotting!")
}
fn check_sys(
Expand Down