Skip to content

fs(readfile): merge open and fstat #45314

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.

Sign up for GitHub

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

Closed
tony-go wants to merge 5 commits into from
Closed

fs(readfile): merge open and fstat #45314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2df711
fs: merge open and stat for readfile
tony-go Nov 3, 2022
d86bb8b
fs: add path in ENOENT error message
tony-go Nov 4, 2022
99ab88c
fs: delete OpenFileCtx and custom AfterStat callback
tony-go Nov 7, 2022
b3ad214
fs: rename OpenFile to OpenAndFstat
tony-go Nov 9, 2022
61222b3
revert: unrelated change
tony-go Jun 5, 2023
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
9 changes: 7 additions & 2 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,12 +400,17 @@ function readFile(path, options, callback) {
const flagsNumber = stringToFlags(options.flag, 'options.flag');
path = getValidatedPath(path);

const fdSlot = new Buffer.alloc(1);
const req = new FSReqCallback();
req.context = context;
req.oncomplete = readFileAfterOpen;
binding.open(pathModule.toNamespacedPath(path),
req.oncomplete = function (err, stats) {
context.fd = fdSlot[0];
readFileAfterStat.apply({ context }, [err, stats]);
}
binding.openFile(pathModule.toNamespacedPath(path),
flagsNumber,
0o666,
fdSlot,
req);
}

Expand Down
61 changes: 61 additions & 0 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,31 @@ void AfterStat(uv_fs_t* req) {
}
}

void OpenAndFstatAfterOpen(uv_fs_t* open_req) {
FSReqBase* req_wrap = FSReqBase::from_req(open_req);
int result = static_cast<int>(open_req->result);

FS_ASYNC_TRACE_END1(
open_req->fs_type, req_wrap, "result", result)

if (result < 0) {
FSReqAfterScope after(req_wrap, open_req);

@addaleax addaleax Nov 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This calls uv_fs_req_cleanup() here. Where is that function called if result >= 0, i.e. when this conditional is not hit?

@tony-go tony-go Nov 9, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I probably misunderstood something, but as we pass req_wrap->req() to the fstat function, I thought that we should use the FSReqAfterScope only if we want to exit:

  • In this case (when there is a problem with open
  • In the fstat callback AfterStat

Am I correct?

@addaleax addaleax Nov 9, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, FSReqAfterScope would destroy the req_wrap object; however, calling uv_fs_req_cleanup() is also an important part of what the function does, and if you don’t call it through FSReqAfterScope, you should call it directly after the libuv call.

req_wrap->Reject(
UVException(req_wrap->env()->isolate(), result, "open", nullptr, open_req->path));

return;
}

char* fd = static_cast<char *>(open_req->data);
fd[0] = result;

@addaleax addaleax Nov 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

char is not a viable type for storing a file descriptor since it has a much smaller range


FS_ASYNC_TRACE_BEGIN0(UV_FS_FSTAT, req_wrap)
uv_fs_fstat(req_wrap->env()->event_loop(),
req_wrap->req(),
open_req->result,
AfterStat);
}

void AfterStatFs(uv_fs_t* req) {
FSReqBase* req_wrap = FSReqBase::from_req(req);
FSReqAfterScope after(req_wrap, req);
Expand Down Expand Up @@ -2069,6 +2094,41 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
}
}

static void OpenAndFstat(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

const int argc = args.Length();
CHECK_GE(argc, 4);

BufferValue path(env->isolate(), args[0]);
CHECK_NOT_NULL(*path);

CHECK(args[1]->IsInt32());
const int flags = args[1].As<Int32>()->Value();

CHECK(args[2]->IsInt32());
const int mode = args[2].As<Int32>()->Value();

CHECK(Buffer::HasInstance(args[3]));
Local<Object> buffer_obj = args[3].As<Object>();
char* fd_buffer = Buffer::Data(buffer_obj);

FSReqBase* req_wrap = GetReqWrap(args, 4, false);
if (req_wrap != nullptr) {
req_wrap->set_is_plain_open(true);
// Note: set fd in context
req_wrap->req()->data = fd_buffer;

FS_ASYNC_TRACE_BEGIN0(UV_FS_OPEN, req_wrap)
uv_fs_open(env->event_loop(),

@addaleax addaleax Nov 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These should be using AsyncCall, otherwise be calling req_wrap->Dispatch(), not the libuv function directly (this is probably the cause of the crash you were seeing

@tony-go tony-go Nov 9, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I already tried AsyncCall, but as he needs args as an argument, it was not convenient to use it in the open callback.

But I'll try it again and yeah, maybe using req_wrap->Dispatch() and req_wrap->Init() is probably a good alternative ^^

@tony-go tony-go Nov 9, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also remember that I've tried (in another branch) to change AsyncCall to chain multiple fs calls. But as AsyncCall was widely used, I thought using something standalone could reduce the impact on other fs APIs. At least to run a benchmark and see if it is worth it.

req_wrap->req(),
*path,
flags,
mode,
OpenAndFstatAfterOpen);
}
}

static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
Realm* realm = Realm::GetCurrent(args);
BindingData* binding_data = realm->GetBindingData<BindingData>();
Expand Down Expand Up @@ -3292,6 +3352,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {

registry->Register(Close);
registry->Register(Open);
registry->Register(OpenAndFstat);
registry->Register(OpenFileHandle);
registry->Register(Read);
registry->Register(ReadBuffers);
Expand Down