Skip to content

fs: handle fixes#45909

Closed
ronag wants to merge 5 commits into
nodejs:mainfrom
nxtedition:fixes-123
Closed

fs: handle fixes#45909
ronag wants to merge 5 commits into
nodejs:mainfrom
nxtedition:fixes-123

Conversation

@ronag

@ronag ronag commented Dec 19, 2022

Copy link
Copy Markdown
Member

Minor fixes found while reviewing code. I don't plan on working further on this so if someone wants to finish up with the test be my guest.

@ronag ronag added the good first issue Issues that are suitable for first-time contributors. label Dec 19, 2022
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Dec 19, 2022
ronag referenced this pull request Dec 19, 2022
@mmomtchev @aduh95
stream: add FileHandle support to Read/WriteStream
0fd121e 
Support creating a Read/WriteStream from a
FileHandle instead of a raw file descriptor
Add an EventEmitter to FileHandle with a single
'close' event.

Fixes: #35240

PR-URL: #35922
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ronag ronag mentioned this pull request Dec 20, 2022
Closed
@ronag ronag requested a review from aduh95 December 25, 2022 11:12
@ronag ronag marked this pull request as ready for review December 25, 2022 11:12
@ronag ronag removed the good first issue Issues that are suitable for first-time contributors. label Dec 25, 2022
@ronag

ronag commented Dec 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

@nodejs/fs @mcollina

ronag
ronag commented Dec 25, 2022
View reviewed changes
Comment thread lib/internal/fs/streams.js
throw new ERR_METHOD_NOT_IMPLEMENTED('open()');
},
close: (fd, cb) => {
handle.off('close', closeHandler);

@ronag ronag Dec 25, 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.

Avoid keeping stream alive until file handle is closed.

ronag
ronag commented Dec 25, 2022
View reviewed changes
Comment thread lib/internal/fs/streams.js
() => cb(), cb);
if (handle[kFd] !== -1) {
// Someone else has a ref to the handle.
process.nextTick(cb);

@ronag ronag Dec 25, 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.

stream can finish teardown even if file handle isn't closed, if it has refs

ronag
ronag commented Dec 25, 2022
View reviewed changes
Comment thread lib/internal/fs/promises.js
() => { this[kClosePromise] = undefined; }
() => {
this[kClosePromise] = undefined;
this.emit('close');

@ronag ronag Dec 25, 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.

Only emit close after actually closing.

@ronag ronag added the review wanted PRs that need reviews. label Jan 1, 2023
@Slayer95

Slayer95 commented Mar 3, 2023

Copy link
Copy Markdown
Contributor

I'm afraid this PR flew under people's radar.

aduh95
aduh95 reviewed Mar 3, 2023
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM except the test failures

aduh95
aduh95 reviewed Mar 3, 2023
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM except the test failures

@ronag ronag closed this Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ronag @Slayer95 @aduh95 @nodejs-github-bot