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

add server.shutdown() method to SocketServer #41934

Closed
phr mannequin opened this issue May 2, 2005 · 9 comments
Closed

add server.shutdown() method to SocketServer #41934

phr mannequin opened this issue May 2, 2005 · 9 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@phr
Copy link
Mannequin

phr mannequin commented May 2, 2005

BPO 1193577
Nosy @smontanaro, @akuchling
Superseder
  • bpo-2302: Uses of SocketServer.BaseServer.shutdown have a race
  • Files
  • issue1193577.patch: diff from current trunk, SocketServer and test_socketserver
  • polling_shutdown.patch: werneck's patch, merged up to r61161
  • polling_shutdown.patch: timeouts reorganized; shutdown blocks
  • polling_shutdown.patch: +documentation
  • 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 2008-03-16.17:20:30.103>
    created_at = <Date 2005-05-02.03:04:36.000>
    labels = ['type-feature', 'library']
    title = 'add server.shutdown() method to SocketServer'
    updated_at = <Date 2008-03-16.17:20:30.102>
    user = 'https://bugs.python.org/phr'

    bugs.python.org fields:

    activity = <Date 2008-03-16.17:20:30.102>
    actor = 'jyasskin'
    assignee = 'jyasskin'
    closed = True
    closed_date = <Date 2008-03-16.17:20:30.103>
    closer = 'jyasskin'
    components = ['Library (Lib)']
    creation = <Date 2005-05-02.03:04:36.000>
    creator = 'phr'
    dependencies = []
    files = ['9553', '9579', '9581', '9586']
    hgrepos = []
    issue_num = 1193577
    keywords = ['patch']
    message_count = 9.0
    messages = ['54518', '62804', '62994', '63170', '63172', '63177', '63185', '63345', '63587']
    nosy_count = 7.0
    nosy_names = ['skip.montanaro', 'akuchling', 'phr', 'pilcrow', 'jyasskin', 'zanella', 'werneck']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = '2302'
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1193577'
    versions = ['Python 2.6', 'Python 3.0']

    @phr
    Copy link
    Mannequin Author

    phr mannequin commented May 2, 2005

    First of all the daemon_threads property on socket
    servers should be added as an optional arg to the
    server constructor, so you can say:
    TCPServer(..., daemon_threads=True).serve_forever()

    instead of

        temp = TCPServer(...)
        temp.daemon_threads=True
        temp.serve_forever

    Secondly there should be a way to STOP the server, once
    you've started serving forever! A request handler
    should be able to say

       self.server.stop_serving()

    This would work by setting a flag in the server object.
    The serve_forever loop would use select with a timeout
    (default 0.5 sec, also settable as an optional arg to
    the server constructor) or alternatively set a timeout
    on the socket object. So it would block listening for
    new connections, but every 0.5 sec it would check the
    stop_serving flag and break out of the loop if the flag
    was set.

    This method was suggested by Skip Montanaro in a clpy
    thread about how to stop SocketServer and it's useful
    enough that I think it should be part of the standard
    impl. I can make a patch if it's not obvious how to do it.

    @phr phr mannequin assigned smontanaro May 2, 2005
    @phr phr mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 2, 2005
    @phr phr mannequin assigned smontanaro May 2, 2005
    @phr phr mannequin added the stdlib Python modules in the Lib dir label May 2, 2005
    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented Feb 23, 2008

    This is getting in my way, so I'll take a look at it. I'm planning to
    model the shutdown API after
    http://java.sun.com/javase/6/docs/api/java/util/concurrent/ExecutorService.html.
    The serve_forever->shutdown interval should probably also be available
    through a context manager.

    @jyasskin jyasskin mannequin assigned jyasskin and unassigned smontanaro Feb 23, 2008
    @werneck
    Copy link
    Mannequin

    werneck mannequin commented Feb 25, 2008

    I had some code here to do the exact same thing with XML-RPC server. The
    patch adds the feature to SocketServer, with a server.shutdown() method
    to stop serve_forever(), and change the unittest to use and test the
    feature.

    I disagree on adding the daemon_threads arg as a keyword to TCPServer
    since that's a feature of ThreadingMixIn

    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented Mar 1, 2008

    Polling isn't a great way to handle shutdown, since it wastes CPU time
    and decreases responsiveness, but it's also easy and my attempt to avoid
    it isn't getting anywhere, so I'm planning to fix up your patch a bit
    and commit it. Thanks!

    I've merged your patch with my conflicting change in the trunk and
    re-attached it.

    Two things:

    1. This patch may interfere with the existing timeout in await_request.
      We may be able to re-use that instead of having two select statements.

    2. I believe it's important to provide a way to block until the server
      isn't accepting any more requests and to block until all active requests
      are finished running. If the active requests depend on other bits of the
      system, blocking until they're done would help them terminate
      gracefully. It would also be useful to give users a more proactive way
      to kill active requests, perhaps by listing the handler objects
      associated with them (or their PIDs for forking servers). It's
      surprisingly complicated to avoid race conditions in these
      implementations, especially without support from the Server object.

    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented Mar 1, 2008

    Also, Pedro's argument against a daemon_threads argument to TCPServer
    convinces me. I think we can add it in ThreadingMixIn.__init__ once this
    whole hierarchy is switched over to new-style classes. That's already
    done in 3.0, but I don't know what it would affect in 2.6. If anyone
    wants to keep pushing it, would you open a new bug?

    @jyasskin jyasskin mannequin changed the title add server.shutdown() method and daemon arg to SocketServer add server.shutdown() method to SocketServer Mar 1, 2008
    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented Mar 2, 2008

    It seems that .await_request() was only added a month ago to fix issue
    742598, so it's no great hardship to refactor it again now. Timeouts
    never worked for .serve_forever() because the try/except in
    .handle_request() turned their exception into a plain return, which
    didn't stop .server_forever() from looping. Timeouts also seem to be
    unnecessary in a situation in which shutdown works. Shutdown can only be
    called from a separate thread, and when you have more threads you can
    also do periodic tasks in them.

    So I've made that explicit: the .serve_forever/shutdown combination
    doesn't handle timeouts. [This has nothing to do with the fact that it
    takes three times as much code to handle them. Don't look at the excuse
    behind the curtain. ;)]

    This patch needs some more documentation and a NEWS entry before it gets
    submitted, but I want to check that everyone would be happy with the
    concept.

    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented Mar 2, 2008

    I'll submit the attached patch in a couple days unless I get comments.

    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented Mar 7, 2008

    Hearing no objections, I've submitted this as r61289.

    @jyasskin
    Copy link
    Mannequin

    jyasskin mannequin commented Mar 16, 2008

    So this has a race. See bpo-2302 to discuss a fix.

    @jyasskin jyasskin mannequin closed this as completed Mar 16, 2008
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 9, 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