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

Jump to bottom

fs: do not emit 'finish' before 'open' on writing empty file #29930

Closed
wants to merge 1 commit into from
Closed

fs: do not emit 'finish' before 'open' on writing empty file #29930

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Oct 11, 2019 •
edited
Loading

'finish' could previously be emitted before the file has been
created when ending a write stream without having written any
data.

Refs: expressjs/multer#238

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 11, 2019
This was referenced Oct 11, 2019
Closed
Closed
@ronag ronag force-pushed the fs-fix-write-stream-open-final branch from 936bd44 to c0ba9a2 Compare October 11, 2019 10:19
@ronag
Copy link
Member Author

ronag commented Oct 11, 2019

@Trott @mcollina

@mcollina
Copy link
Member

The commit has the wrong subsystem prefix. It’s not stream, but rather fs.

@Trott
Copy link
Member

Trott commented Oct 11, 2019

@nodejs/fs

@Trott Trott changed the title stream: don't emit 'finish' before 'open' on write empty file fs: do not emit 'finish' before 'open' on writing empty file Oct 11, 2019
@Trott
Copy link
Member

Trott commented Oct 11, 2019

The commit has the wrong subsystem prefix. It’s not stream, but rather fs.

I tried to amend the commit message but don't have permission to the nxtedition/node.git repo. But I did change the title of this PR to what I would recommend the commit message be changed to.

Trott
Trott approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with commit messages/subsytem changed to fs (and assuming no CI surprises).

Will run CITGM too to make sure this fixes the multer issue and doesn't unexpectedly create issues somewhere else.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott
Copy link
Member

Trott commented Oct 11, 2019

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2049/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 11, 2019
@ronag
fs: don't emit 'finish' before 'open' on write empty file
f341a47 
'finish' could previously be emitted before the file has been
created when ending a write stream without having written any
data.

Refs: expressjs/multer#238
@ronag ronag force-pushed the fs-fix-write-stream-open-final branch from c0ba9a2 to f341a47 Compare October 11, 2019 13:54
@ronag
Copy link
Member Author

ronag commented Oct 11, 2019

amended commit msg

mcollina
mcollina approved these changes Oct 11, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added this to the 13.0.0 milestone Oct 11, 2019
@Trott
Copy link
Member

Trott commented Oct 12, 2019

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2049/

CITGM results look great. No multer failures anymore and no new unexpected failures.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 12, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/25979/ ✔️

Trott pushed a commit that referenced this pull request Oct 13, 2019
@ronag @Trott
fs: do not emit 'finish' before 'open' on write empty file
ba45367 
'finish' could previously be emitted before the file has been
created when ending a write stream without having written any
data.

Refs: expressjs/multer#238

PR-URL: #29930
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 13, 2019

Landed in ba45367. Thanks!

@Trott Trott closed this Oct 13, 2019
@Trott Trott mentioned this pull request Oct 15, 2019
Merged
targos pushed a commit that referenced this pull request Oct 18, 2019
@ronag @targos
fs: do not emit 'finish' before 'open' on write empty file
8ebc945 
'finish' could previously be emitted before the file has been
created when ending a write stream without having written any
data.

Refs: expressjs/multer#238

PR-URL: #29930
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 21, 2019
Merged
@boneskull
Copy link
Contributor

It looks like this may be causing some test failures in userland: see sindresorhus/copy-file#38

https://travis-ci.org/sindresorhus/cp-file/jobs/602461047#L280

In this case, errno is undefined whereas before it was -5.

@boneskull
Copy link
Contributor

previously the first error emitted from the fs readable stream was EIO; now it's ERR_STREAM_WRITE_AFTER_END. going to guess that's because the latter now comes out of "open" and the former out of "finish"

given this test is doing some monkeypatching in its harness, we can probably just work around that, but wanted to make sure this was intended?

@ronag
Copy link
Member Author

ronag commented Oct 24, 2019

@boneskull: I'm not sure I'm following. Can you provide a simple example?

@boneskull
Copy link
Contributor

@ronag I can't really, as it's not my code and I don't really understand the intent. But I can tell you that previous to Node.js v12.13.0, the first error emitted from a writable stream in this test case was one w/ code EIO; and now it's one with ERR_STREAM_WRITE_AFTER_END.

https://github.com/sindresorhus/cp-file/blob/master/test/async.js#L244

@boneskull boneskull mentioned this pull request Oct 24, 2019
Merged
@mcollina
Copy link
Member

@boneskull would you mind opening a new issue in this repo? Would you mind confirming that Node 12.12.0 was working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
13.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@ronag @mcollina @Trott @nodejs-github-bot @boneskull @addaleax @Fishrock123 @lpinca