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

Destroy socket on client close #250

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

martijnimhoff
Copy link
Contributor

Fixes: #249

@martijnimhoff
Copy link
Contributor Author

We can actually use .destroy() without .end().

Rationale for using destroy:

The docs say .end half closes the socket and .destroy will release any internal resources. I tried it with just a .destroy and checked the FTP logs to see what happens. I still get the following:

Thu Feb 22 14:57:07 2024 FTP command: Client "172.17.0.1", "QUIT"
Thu Feb 22 14:57:07 2024 FTP response: Client "172.17.0.1", "221 Goodbye."

It seems the connection is still closed gracefully? Therefore I think it is safe to use .destroy.

(I've also tested if this works when closing a client and then using access to re-establish the connecting again, I had no issues there)

@martijnimhoff
Copy link
Contributor Author

martijnimhoff commented Feb 26, 2024

I've also added a prepare command. In this way I can install my fork without having to publish it. Based on this answer:
https://stackoverflow.com/a/57829251/5688047

@patrickjuchli
Copy link
Owner

Thanks for this! I agree, let's go with destroy immediately. Good to keep the doNothing handler for any errors.

For future PRs: Try to limit your PR to exactly the described issue. You've included some other things. I'll make an exception and merge them as well, but try to avoid that next time/elsewhere.

@patrickjuchli patrickjuchli merged commit 1087e4b into patrickjuchli:master Feb 27, 2024
8 checks passed
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.

Memory leak in Client
2 participants