-
Notifications
You must be signed in to change notification settings - Fork 349
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
do not remove the uds socket on pause #442
base: master
Are you sure you want to change the base?
Conversation
DO you think the CI error, might be related to this ? |
No idea, usually I ignore linker errors if they're not happening on all versions. It'll go away eventually. |
You should use And my question in #97 still stands. The desired behaviour is still unclear to me. Does the file need to be cleared on server shutdown? I'm asking this because the accept thread is notified to shutdown in non blocking manner and other part of the server would race to exit the program which means the code you write may not get a chance to run at all. This lead to two possible solution:
Either is better than an inconsistent removal. |
The server should try to always clean is file when it is closing. This is what most server does and it leave the system in a cleaner state. However, we can't be ensure the socket file will be clean, because unlike a tcp socket, the kernel will not remove the file when sending a SIGKILL. So the server should handle this on startup (right now it does). Does implementing the |
Drop trait is the idiomatic way of clean up on resource go out of scope. Drop is not guaranteed to run either it's just more clear on the purpose and it's an implicit called compared to a homebrew method. So it's more about keep the code clean and readable. As for block and wait for accept thread to exit you can look into this part of the code: actix-net/actix-server/src/server.rs Line 247 in 7248131
There are two possible ways to achieve a blocking cleanup:
|
I made a PR for solution 1 in previous reply. If you don't mind you can rebase on it when it's accepted. If you like to implement a different way in your PR it's also fine. |
Thanks for your suggestion it is clear, I will look at your PR and look to rebase on it. I also re-think about if we should clean the file and I think their is some case where the server is not the owner of the file. One use case I have in mind is systemd socket activation, you receive a file descriptor not path, so you should let systemd manage the file. Maybe we should clean the file only when the server receive a path, but if we receive a socket, we should not remove it since the server is not technically the owner... I will look at the different way to bind the server (I do not put too much attention the last time). |
By looking around, I think we should leave the socket here. It would be cleaner to remove it on clean shutdown, but it will not be armful to leave it here. In fact this is what systemd does and even syslog. For a client this is not a problem because if you try to connect on a close socket, even if the file exist, you receive a Connection refused. If we need more we can always try to do the cleanup later, but for now, leaving the socket file will close the issue for everyone. |
This is still affecting us, any news about this ? This prevent the use of systemd socket activation. |
temporarily marking as draft due to conflicts |
PR Type
Bug Fix
PR Checklist
Check your PR fulfills the following:
Overview
Add a terminate function to the listener socket so the socket file is not remove in the deregister, but really when it is necessary. For a tcp socket, we have nothing to do and in the deregister, there is nothing left special to do for a UDS.
Closes #364 #97