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

Defensively cleanup streams #2231

Closed
wants to merge 4 commits into from
Closed

Conversation

andrewscfc
Copy link

@andrewscfc andrewscfc commented Nov 3, 2022

Fix/workaround for #2100

This PR in local testing prevented file descriptors from leaking as demonstrated by running the lsof command and not seeing file descriptors increase.

The fix introduces a mechanism to capture all streams created in an array and ensures all are destroyed when _cleanupStream is called.

The code reads to me like there should only ever be one file stream in scope at any time but for some reason we find multiple are created before they are closed.

This is a workaround really and it would be better to find the source of this issue; streams are being created but not cleaned up, this fix just ensures the resource leak does not occur.

@andrewscfc andrewscfc changed the title destroy stream explicitly Defensively cleanup streams Nov 4, 2022
@DABH
Copy link
Contributor

DABH commented Jan 8, 2024

This is a nice workaround :) But since I think we wouldn't want something like this being the official way of doing things (it's obviously just a bandaid), I'm going to go ahead and close this PR. However, I'm absolutely going to keep it in mind as a reference while working on issues like #2100 (and what sounds like is going to be a new related issue, since #2301 seems to have solved the specific MWE of #2100). So this was not in vain -- but making a bit of effort to go through and clean up some old open PRs on winston. Thank you very much for your contributions!

@DABH DABH closed this Jan 8, 2024
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.

3 participants