classification
Title: socketserver.BaseServer.close_server should stop serve_forever
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: martin.panter, palaviv
Priority: normal Keywords: patch

Created on 2016-02-19 17:00 by palaviv, last changed 2016-02-23 18:38 by palaviv. This issue is now closed.

Files
File name Uploaded Description Edit
socketserver_close_stop_serve_forever.patch palaviv, 2016-02-19 17:00 review
socketserver_close_stop_serve_forever2.patch palaviv, 2016-02-20 10:15 review
Messages (5)
msg260524 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-19 17:00
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.
IMO you should be able to call server_close at any time and expect it to stop the serve_forever. Maybe even adding a block option to server_close that will wait on the server_forever if it's running (waiting for issue 12463 to resolve before doing this).
Added a patch that closes serve_forever if server_close is called.
msg260532 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-19 22:41
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 Issue 13749, and maybe Issue 23430 would help by allowing exceptions like SystemExit to stop the server.
msg260564 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-20 10:15
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 don't think that this should be an enhancement as what i try to do is return the behavior as it was before the change to the selectors module. When closing the socket using server_close at the next serve_forever loop you would have gotten ValueError for bad file descriptor to select. In the current implementation we actually keep on pulling on a already free resource.
I know that the user should call shutdown before the server_close but i think that as you said the behavior should simulate closed file. When someone will try to use any other function of a closed file he will receive a ValueError.
msg260669 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-22 10:50
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).
msg260741 - (view) Author: Aviv Palivoda (palaviv) * Date: 2016-02-23 18:38
I understand the problem and why this patch should be rejected.
History
Date User Action Args
2016-02-23 18:38:51palavivsetstatus: open -> closed
resolution: rejected
messages: + msg260741
2016-02-22 10:50:36martin.pantersetmessages: + msg260669
2016-02-20 10:15:19palavivsetfiles: + socketserver_close_stop_serve_forever2.patch

messages: + msg260564
2016-02-19 22:41:38martin.pantersettype: enhancement

messages: + msg260532
nosy: + martin.panter
2016-02-19 17:00:17palavivcreate