Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

force socket to be closed on destroy #785

Merged
merged 1 commit into from
Feb 25, 2022
Merged

Conversation

dfit99
Copy link

@dfit99 dfit99 commented Feb 11, 2022

This will force destroy a socket after calling destroy.
Fixing the file descriptor leak issue, see #605

@UziTech
Copy link
Member

UziTech commented Feb 11, 2022

Can you write a test for this?

@dfit99
Copy link
Author

dfit99 commented Feb 11, 2022

Hi Tony,

Thanks for the quick reply, do you have any suggestions on where to add the tests to and what exactly to test?
There aren't any contribution guidelines I could find.

@UziTech
Copy link
Member

UziTech commented Feb 11, 2022

You can write the test in /test/client.test.js. Try to find a test that does something similar and copy it.

@dfit99
Copy link
Author

dfit99 commented Feb 15, 2022

I added some tests

@dfit99 dfit99 marked this pull request as ready for review February 15, 2022 15:29
Copy link
Member

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

LGTM

@dfit99
Copy link
Author

dfit99 commented Feb 16, 2022

@UziTech Thank you for approving.
Can you please merge this in -- it says "Only those with [write access]) to this repository can merge pull requests."

@dfit99
Copy link
Author

dfit99 commented Feb 25, 2022

@UziTech Just following up here, are there plans to merge this in?
I ask because this is a critical issue for my team (the memory leaked caused some of our production processes to crash) and would ideally not have to use a forked version of this library.

Sorry for rushing you and thanks again for reviewing my PR :)

@jsumners
Copy link
Member

@UziTech Just following up here, are there plans to merge this in? I ask because this is a critical issue for my team (the memory leaked caused some of our production processes to crash) and would ideally not have to use a forked version of this library.

Sorry for rushing you and thanks again for reviewing my PR :)

Releases are issued according to the maintainers's available time and desire to work on the project.

@jsumners jsumners merged commit e3318a1 into ldapjs:master Feb 25, 2022
@jsumners
Copy link
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants