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

src: cleanup uv_fs_t regardless of success or not #38996

Closed
wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jun 10, 2021

  1. Cleanup uv_fs_t regardless of success or not.
  2. Prevent from accessing members of uv_fs_t after cleanup.

This is a quite recent change, so I'm requesting original author @joyeecheung 's review.

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 10, 2021
@mmarchini mmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 10, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38562/

  • not ok 2626 parallel/test-worker-cleanexit-with-moduleload

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 11, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38569/

  • ld terminated with signal 9

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2021
@nodejs-github-bot
Copy link
Collaborator

@@ -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.

@legendecas legendecas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 16, 2021
@legendecas
Copy link
Member Author

Landed in e4eadb2

legendecas added a commit that referenced this pull request Jun 16, 2021
PR-URL: #38996
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@legendecas legendecas closed this Jun 16, 2021
@legendecas legendecas deleted the util/read-file branch June 16, 2021 14:42
@legendecas legendecas removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 16, 2021
danielleadams pushed a commit that referenced this pull request Jun 21, 2021
PR-URL: #38996
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@danielleadams danielleadams mentioned this pull request Jun 21, 2021
richardlau pushed a commit that referenced this pull request Jul 19, 2021
PR-URL: #38996
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
richardlau pushed a commit that referenced this pull request Jul 20, 2021
PR-URL: #38996
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@richardlau richardlau mentioned this pull request Jul 20, 2021
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
PR-URL: nodejs#38996
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants