Skip to content

Destroy socket on client close#250

Merged
patrickjuchli merged 3 commits into
patrickjuchli:masterfrom
withthegrid:master
Feb 27, 2024
Merged

Destroy socket on client close#250
patrickjuchli merged 3 commits into
patrickjuchli:masterfrom
withthegrid:master

Conversation

@martijnimhoff

Copy link
Copy Markdown
Contributor

Fixes: #249

@martijnimhoff

Copy link
Copy Markdown
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

martijnimhoff commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants