Skip to content

fix: callback with error if SyncWriteStream writeSync failed #47949

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 26, 2023
Merged

fix: callback with error if SyncWriteStream writeSync failed #47949

merged 1 commit into from
Jun 26, 2023

Conversation

killagu
Copy link
Contributor

@killagu killagu commented May 10, 2023

Fixes: #47948

@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 May 10, 2023
@killagu killagu force-pushed the fix/stdout_error branch from 108e831 to c5b5375 Compare May 10, 2023 12:00
@bnoordhuis
Copy link
Member

bnoordhuis commented May 11, 2023 •
edited
Loading

General observation: the way _write() synchronously invokes cb() is wrong(ish). Node normally follows the iron-clad rule that callbacks are always invoked asynchronously, otherwise you can end up overflowing the stack. Ex.:

function again() {
  stream._write(buf, 'utf8', again); // RangeError: Maximum call stack size exceeded 
}

edit: oh nevermind, this is stream.Writable._write() and the streams code takes care of deferring the callback.

@killagu killagu force-pushed the fix/stdout_error branch 3 times, most recently from 3726f80 to 7c8c81f Compare May 11, 2023 07:39
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2023
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced May 13, 2023
Open
Open
@killagu killagu force-pushed the fix/stdout_error branch 2 times, most recently from c38701c to eff6f0f Compare May 14, 2023 01:32
This was referenced May 15, 2023
Open
Open
Open
Open
@killagu
Copy link
Contributor Author

killagu commented May 23, 2023

ping @bnoordhuis

@fengmk2 fengmk2 mentioned this pull request May 30, 2023
Open
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lpinca
Copy link
Member

lpinca commented Jun 21, 2023

Some failures on Windows seems related. @killagu can you take a look?

@killagu
Copy link
Contributor Author

killagu commented Jun 21, 2023

Of course.

@github-actions github-actions bot mentioned this pull request Jun 24, 2023
Open
25 tasks
@killagu
Copy link
Contributor Author

killagu commented Jun 25, 2023

Sorry, I'm still looking for a suitable Windows machine to run Node.js compilation and testing. I will provide an update once there is progress.

@killagu killagu force-pushed the fix/stdout_error branch from 8c4b1af to 7bcb45c Compare June 26, 2023 07:35
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2023
@killagu
Copy link
Contributor Author

killagu commented Jun 26, 2023

@lpinca I've identified the cause. On the Windows platform, it requires manual release of the fd; otherwise, it will block the process from cleaning up the temporary directory upon exiting. Could you please trigger the CI again? Thank you.

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

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

@killagu killagu force-pushed the fix/stdout_error branch from 7bcb45c to 021f107 Compare June 26, 2023 07:41
@killagu
Copy link
Contributor Author

killagu commented Jun 26, 2023

😅 Force push let the ci down fatal: reference is not a tree: 7bcb45c7a81c3bba6e9ffa96754329c9facd4aa4.

@nodejs-github-bot
Copy link
Collaborator

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

lpinca
lpinca approved these changes Jun 26, 2023
View reviewed changes
@lpinca
Copy link
Member

lpinca commented Jun 26, 2023

Also one final request. Can you please change the commit message to something like this?

fs: call the callback with an error if writeSync fails

Thank you.

@killagu
fs: call the callback with an error if writeSync fails
927888b 
Catch SyncWriteStream write file error.

Fixes: nodejs#47948
Signed-off-by: killagu <killa123@126.com>
@killagu killagu force-pushed the fix/stdout_error branch from dd12bf0 to 927888b Compare June 26, 2023 11:15
@killagu
Copy link
Contributor Author

killagu commented Jun 26, 2023 •
edited
Loading

It's updated. Thanks very much for review and lots of help.

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

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

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2023
@nodejs-github-bot nodejs-github-bot merged commit 32eb492 into nodejs:main Jun 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 32eb492

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@killagu @RafaelGSS
fs: call the callback with an error if writeSync fails
19fde63 
Catch SyncWriteStream write file error.

Fixes: #47948
Signed-off-by: killagu <killa123@126.com>
PR-URL: #47949
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@killagu @Ceres6
fs: call the callback with an error if writeSync fails
d25c3bf 
Catch SyncWriteStream write file error.

Fixes: nodejs#47948
Signed-off-by: killagu <killa123@126.com>
PR-URL: nodejs#47949
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@killagu @Ceres6
fs: call the callback with an error if writeSync fails
1ed6d0b 
Catch SyncWriteStream write file error.

Fixes: nodejs#47948
Signed-off-by: killagu <killa123@126.com>
PR-URL: nodejs#47949
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 10, 2023
@killagu @ruyadorno
fs: call the callback with an error if writeSync fails
e52b364 
Catch SyncWriteStream write file error.

Fixes: #47948
Signed-off-by: killagu <killa123@126.com>
PR-URL: #47949
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 10, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
@killagu @ruyadorno
fs: call the callback with an error if writeSync fails
5354af3 
Catch SyncWriteStream write file error.

Fixes: #47948
Signed-off-by: killagu <killa123@126.com>
PR-URL: #47949
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@lpinca lpinca lpinca approved these changes

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

process.stdout may memleak if write file failed.
5 participants
@killagu @bnoordhuis @nodejs-github-bot @lpinca @kvakil