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.

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

src: cleanup uv_fs_t regardless of success or not #38996

Closed
wants to merge 1 commit into from
Closed

src: cleanup uv_fs_t regardless of success or not #38996

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion src/util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,13 @@ int WriteFileSync(v8::Isolate* isolate,

int ReadFileSync(std::string* result, const char* path) {
uv_fs_t req;
auto defer_req_cleanup = OnScopeLeave([&req]() {
uv_fs_req_cleanup(&req);
});
Copy link
Member

Choose a reason for hiding this comment

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

An alternative approach (that may be a bit nicer here) is to use a smart-pointer type of mechanism like we use elsewhere... something like...

struct UvFsReq {
  uv_fs_t req;
  ~UvFsReq() {
    uv_fs_req_cleanup(&req);
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds great, I'll do an update.

Copy link
Member Author

Choose a reason for hiding this comment

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

After several tries, I found that wrapping with a naive data struct did not work well in the case. We have to access members of uv_fs_t and passing the pointer of uv_fs_t back to uv, like:

uv_fs_t req;
uv_fs_open(nullptr, &req, path, O_RDONLY, 0, nullptr);  // <- either we write with `&UvFsReq.req` or using operators to automatically convert (which might not be very straightforward since it implicit converting from data type to pointer type)
if (req.result < 0); // <- accessing member of `uv_fs_t`, either we write with UvFsReq.req.result or explicitly declare the member proxy in UvFsReq like `UvFsReq.result()`.

This is too much for the intent, a OnScopeLeave just fit in well.

As such, I'd believe it is more readable and straightforward to use uv_fs_t here. And there are a lot of precedents in the code base.


uv_file file = uv_fs_open(nullptr, &req, path, O_RDONLY, 0, nullptr);
if (req.result < 0) {
// req will be cleaned up by scope leave.
return req.result;
}
uv_fs_req_cleanup(&req);
Expand All @@ -243,7 +248,7 @@ int ReadFileSync(std::string* result, const char* path) {
const int r =
uv_fs_read(nullptr, &req, file, &buf, 1, result->length(), nullptr);
if (req.result < 0) {
uv_fs_req_cleanup(&req);
// req will be cleaned up by scope leave.
return req.result;
}
uv_fs_req_cleanup(&req);
Expand Down