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

Socket not closable before program end. #337

Closed
OmegaMetor opened this issue Jun 25, 2022 · 3 comments · Fixed by #422
Closed

Socket not closable before program end. #337

OmegaMetor opened this issue Jun 25, 2022 · 3 comments · Fixed by #422
Labels
area: protocol Related to underlying networking protocol marker: good first issue Good for newcomers type: bug Something isn't working

Comments

@OmegaMetor
Copy link

When doing something like

server = JavaServer.lookup("example.com:25565")
status = server.status()

There does not appear to be a way to close the socket connection (normally done on program end). Because of this, large amounts of calls to get server data from a queue will run into the error "Too many open files", the files being the sockets. The only way I can currently see to close the socket is to end the program. If there is another way to do this, please correct me.

@ItsDrike ItsDrike added type: bug Something isn't working area: API Related to core API of the project labels Jun 26, 2022
@ItsDrike
Copy link
Member

The socket for our custom connection classes should be automatically getting closed on that instance getting garbage collected using __del__. However I agree that this isn't a good way of doing it, and we should instead be closing the socket explicitly. This is because in some implementations of python (this might even include CPython, but I'm not completely sure), it isn't guaranteed that __del__ will always run on that object's non-explicit deletion (garbage collection).

There is no way to close it for you, since after you ran the status, because the connection is created within the status function, and gets out of scope once the function ends. However it should be an easy fix to just make a connection.socket.close call after the status (or query, etc.) was obtained. For a temporary fix, you could make a child class to JavaServer and override the logic of status if you can't wait for another release.

For the library, this should be an easy fix, as we can just explicitly close the connection once we obtained the status (or query, etc.) by directly calling connection.socket.close, or perhaps ideally by adding a close method on our custom connection class to propagate this socket.close call.

@kevinkjt2000
Copy link
Contributor

We could also add enter and exit functions to enable with statements.

@ItsDrike ItsDrike added the marker: good first issue Good for newcomers label Oct 3, 2022
@ItsDrike
Copy link
Member

ItsDrike commented Oct 13, 2022

Just found an error silently reported by pytest in one of our workflows regarding to this:

image

This error didn't cause the workflow to fail, as exceptions raised from __del__ don't actually affect anything on runtime, however it's a good example on why putting things to __del__ is generally a bad practice. In this error, the asyncio event loop was already closed when we tried to close a connection socket.

I think this __del__ logic should be completely removed and replaced here, instead of just additionally adding context manager support. Since connections are only used internally anyway, it shouldn't require deprecation and it should be an easy fix.

Also, this issue should probably be somehow marked as having somewhat higher priority, as without this getting fixed, using mcstatus in places that require opening up many connections may end up causing this very unintuitive error message of "too many open files".

@ItsDrike ItsDrike added area: protocol Related to underlying networking protocol and removed area: API Related to core API of the project labels Oct 13, 2022
PerchunPak added a commit to PerchunPak/mcstatus that referenced this issue Oct 16, 2022
PerchunPak added a commit to PerchunPak/mcstatus that referenced this issue Oct 24, 2022
PerchunPak added a commit to PerchunPak/mcstatus that referenced this issue Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: protocol Related to underlying networking protocol marker: good first issue Good for newcomers type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants