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
socketserver.BaseServer.close_server should stop serve_forever #70580
Comments
Currently if you call server_close you only close the socket. If we called serve_forever and then call server_close without calling shutdown the serve_forever loop keep running. Before using the selectors module for doing the poll we would have had exception thrown from the select (The socket fd is -1) in serve_forever. |
I thought the purpose of server_close() was to clean up resources after you have finished calling serve_forever() or handle_request(). Just like you call close() on a file after you have finished reading or writing it. If you try to read or write a closed file, that is a programmer error. This proposal sounds like a new feature, but you are overloading or redefining the purpose of server_close(). From the test case I presume you intend to use server_close() in a separate thread from the thread running serve_forever(). But closing a file descriptor while it is being used in another thread does not seem robust to me. It could cause serve_forever() to raise EBADF. In the worst case, consider what happens if an unrelated third thread makes the server’s file descriptor valid again by opening a file. What is your use case? If you want to use multithreading, why can’t you use the shutdown() method? For a single-threaded server, maybe see bpo-13749, and maybe bpo-23430 would help by allowing exceptions like SystemExit to stop the server. |
I see your point about the server_close purpose and I changed the patch to simulate the behavior of closed file (raising ValueError when operating on closed file). |
I suggest to reject this. Checking a flag before using a shared file descriptor is a recipe for a race condition, and in this case, unspecified behaviour: <http://man7.org/linux/man-pages/man2/select.2.html#NOTES\> (applies to select and poll). |
I understand the problem and why this patch should be rejected. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: