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 file descriptor leak in backup #536

Merged
merged 1 commit into from Dec 15, 2022
Merged

fix file descriptor leak in backup #536

merged 1 commit into from Dec 15, 2022

Conversation

emlaver
Copy link
Contributor

@emlaver emlaver commented Dec 9, 2022

Checklist

  • Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1
  • Added tests for code changes or test/build only changes
  • Updated the change log file (CHANGES.md|CHANGELOG.md) or test/build only changes
  • Completed the PR template below:

Description

Replace tmp.fileSync().name in apiDefaults with tmp.tmpNameSync() to discard descriptor of temporary file during backup.

fixes #535

Approach

Update tmp call in includes/config.js

Schema & API Changes

Security and Privacy

Testing

Used this gist to verify that file descriptors are discarded once backup completes.

Monitoring and Logging

@emlaver emlaver self-assigned this Dec 9, 2022
@emlaver emlaver added this to the 2.next milestone Dec 9, 2022
@eiri
Copy link
Member

eiri commented Dec 15, 2022

To be honest I can't see how this fixes the leaked file descriptors, consider that fileSync is using tmpNameSync under the hood. Do you have an explanation why this works?

As an alternative I'd suggest to use tmp.fileSync({ discardDescriptor: true }) instead, as described in https://github.com/raszi/node-tmp/tree/master#controlling-the-descriptor or go an extra mile and use detachDescriptor and manage the descriptor closing at backup done callback, though that sounds like a hustle.

@ricellis
Copy link
Member

Do you have an explanation why this works?

tmpNameSync just returns a name, fileSync uses that name to open the file. We actually have no need to open the file when processing the config, we open it later by creating a read stream anyway.

@eiri
Copy link
Member

eiri commented Dec 15, 2022

So this is essentially using fstat vs fopen? I'd expect fd to still be allocated, tbh, so we'll win on memory and probably op speed, but not on fd count reduce, but I'm not 100% sure, maybe system takes care of that.

Ok, thanks for the clarification, no harm in using this.

@emlaver emlaver merged commit 3377357 into main Dec 15, 2022
@emlaver emlaver deleted the 535-file-desc-leak branch December 15, 2022 16:25
@emlaver emlaver mentioned this pull request Dec 15, 2022
@ricellis ricellis mentioned this pull request Dec 19, 2023
3 tasks
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.

File descriptor leak
3 participants