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

socketserver.BaseServer.close_server should stop serve_forever #70580

Closed
palaviv mannequin opened this issue Feb 19, 2016 · 5 comments
Closed

socketserver.BaseServer.close_server should stop serve_forever #70580

palaviv mannequin opened this issue Feb 19, 2016 · 5 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@palaviv
Copy link
Mannequin

palaviv mannequin commented Feb 19, 2016

BPO 26392
Nosy @vadmium, @palaviv
Files
  • socketserver_close_stop_serve_forever.patch
  • socketserver_close_stop_serve_forever2.patch
  • 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:

    assignee = None
    closed_at = <Date 2016-02-23.18:38:51.025>
    created_at = <Date 2016-02-19.17:00:16.995>
    labels = ['type-feature', 'library']
    title = 'socketserver.BaseServer.close_server should stop serve_forever'
    updated_at = <Date 2016-02-23.18:38:51.023>
    user = 'https://github.com/palaviv'

    bugs.python.org fields:

    activity = <Date 2016-02-23.18:38:51.023>
    actor = 'palaviv'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-02-23.18:38:51.025>
    closer = 'palaviv'
    components = ['Library (Lib)']
    creation = <Date 2016-02-19.17:00:16.995>
    creator = 'palaviv'
    dependencies = []
    files = ['41971', '41978']
    hgrepos = []
    issue_num = 26392
    keywords = ['patch']
    message_count = 5.0
    messages = ['260524', '260532', '260564', '260669', '260741']
    nosy_count = 2.0
    nosy_names = ['martin.panter', 'palaviv']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue26392'
    versions = ['Python 3.5', 'Python 3.6']

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 19, 2016

    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 bpo-12463 to resolve before doing this).
    Added a patch that closes serve_forever if server_close is called.

    @palaviv palaviv mannequin added the stdlib Python modules in the Lib dir label Feb 19, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Feb 19, 2016

    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.

    @vadmium vadmium added the type-feature A feature request or enhancement label Feb 19, 2016
    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 20, 2016

    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.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 22, 2016

    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).

    @palaviv
    Copy link
    Mannequin Author

    palaviv mannequin commented Feb 23, 2016

    I understand the problem and why this patch should be rejected.

    @palaviv palaviv mannequin closed this as completed Feb 23, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant