-
Notifications
You must be signed in to change notification settings - Fork 37
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
Ignore exceptions on socket close #540
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember this being there before, and I removed it because it was completely undocumented. So 2 key things here:
- Why
Exception
? Why not a more specific type? - Add comment explaining why it's necessary, what's causing the issue, as it doesn't seem to happen often.
And ideally, though I don't insist on this, add a test ensuring this handling works as expected in this scenario.
Actually, it was introduced in 2542913 (#56) and was intended to ignore
I am unsure that we care about any exceptions there. I think that it's something wrong with the user OS, but who cares, the socket is used and needs to be only closed. So if the socket failes to close, and we got all info that we need - let's just ignore that.
Looking at #140, I'm unsure that mocking Wow, the project is so big and old... |
This gives me flashbacks to the reason I started maintaining mcstatus. The first PR I merged looked very similar to this. |
Actually it's pretty important to ensure that the OS recognizes the socket as closed, and removes that resource from the process. That's because most kernels have limitations for processes, such as a maximum amount of file descriptors, which sockets do take up (see: #337). This can then end up causing weird errors like "OSError: Too many open files", when creating a new socket connection. Because of that, I'm still not sure it's such a good idea to just blindly ignore all exceptions here, and it's probably worth investigating why this actually occurred, and looking into it a bit more. |
Would it be okay to something, like here? |
Yes! That looks like a much better way to handle this, with the rest of the exceptions still getting raised, but do notice that this handling is done in So I suppose what's going on is that if some internal flag in the socket instance, representing connection status is on. When close is then called, if this flag is on, shutdown is likely getting called before the actual closing, to properly send the disconnection packets. So, I suppose that if it's done manually like this, that flag gets toggled, even if we get an exception later. So calling close afterwards is safe, and that's what then makes the syscall, telling the kernel to remove that socket from the process. I quite like this solution, but I'm not sure if I'm correct about this internal flag, if this is the case, we can use this solution. |
We shutdown socket too, just in other function. |
Then the handling should be done in that function, not in the exit functions, and only on the shutdown. When handling this on the close function itself, if this error occurs, the socket doesn't get closed, the exception is raised from the shutdown line, so we never even get to the line below calling close. The exception from shutdown is handled and ignored, and the socket remains attached to the process. |
4b623ba
to
d38e931
Compare
Sorry for the delay, VRChat is more interesting that I excepected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, we're all just volunteers here.
Looks good, other than the comment in the except line
d38e931
to
8bfbed4
Compare
The #422 overwrote #379 which had the same fix. Although, it was applied to
__del__
method only, and the idea to add this try-except inclose
method was dismissed as we excepted to get this error only during garbage collection (which is not the case since #422).Fixes #538.