Skip to content

Conversation

@legendecas
Copy link
Member

@legendecaslegendecas 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-actionsgithub-actionsbot 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
@mmarchinimmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2021
@github-actionsgithub-actionsbot 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

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2021
@github-actionsgithub-actionsbot 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

@legendecaslegendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2021
@github-actionsgithub-actionsbot 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

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

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

Copy link
MemberAuthor

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
MemberAuthor

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.

@legendecaslegendecas 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
MemberAuthor

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]>
@legendecaslegendecas deleted the util/read-file branch June 16, 2021 14:42
@legendecaslegendecas 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]>
@danielleadamsdanielleadams 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]>
@richardlaurichardlau 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 freeto 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-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@legendecas@nodejs-github-bot@fhinkel@jasnell@joyeecheung@mmarchini