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

fix: make writeFile sync on finish #111

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alfonsograziano
Copy link

@alfonsograziano alfonsograziano commented Jan 3, 2024

I'm investigating on this issue on the node-clinic repo.

Basically, when the script which is profiled by clinic.js calls process.exit(0), the profile file is not written. The file is created but internally it is empty. Changing the writeFile to its synchronous version is fixing this issue locally

@marco-ippolito
Copy link
Contributor

marco-ippolito commented Jan 3, 2024

I think the issue might be caused by writeFile inside a setInterval on the same destination without waiting for the callback.
Quoting Node.js documentation:

It is unsafe to use fs.writeFile() multiple times on the same file without waiting for the callback. For this scenario, fs.createWriteStream() is recommended.

I think it's accettable to use writeFileSync or createWriteStream.

Copy link
Contributor

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

can you add a test?

@alfonsograziano
Copy link
Author

@RafaelGSS I'd love to, but unfortunately the tests are failing for me locally (with and without this change).
At the same time, I already created a test for this case in the clinic-library. Not sure if that's enough :)

@RafaelGSS
Copy link
Contributor

@RafaelGSS I'd love to, but unfortunately the tests are failing for me locally (with and without this change). At the same time, I already created a test for this case in the clinic-library. Not sure if that's enough :)

Did you try using v18?

@RafaelGSS
Copy link
Contributor

You can also cherry pick your commit in the last github tag which is supposed to be passing in CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants