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

cheroot.server: procedure ``serve()`` has detached from ``start()`` #98

Merged
merged 6 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@polymorphm
Contributor

polymorphm commented May 27, 2018

  • What is the related issue number (starting with #)

it is release with issues #68 ("is there any right way to interrupt a started server?")

  • What is the current behavior? (You can also link to an open issue here)

see my next simple example:

# -*- mode: python; coding: utf-8 -*-

from cheroot.wsgi import Server
import signal
import threading

def helloAAA_wsgi(environ, start_response):
    start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
    
    yield 'Hello world! #AAA\n'.encode()

def helloBBB_wsgi(environ, start_response):
    start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
    
    yield 'Hello world! #BBB\n'.encode()

def helloCCC_wsgi(environ, start_response):
    start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
    
    yield 'Hello world! #CCC\n'.encode()

def main():
    signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGINT, signal.SIGTERM})
    
    serverAAA = Server(('127.0.0.1', 8081), helloAAA_wsgi)
    serverBBB = Server(('127.0.0.1', 8082), helloBBB_wsgi)
    serverCCC = Server(('127.0.0.1', 8083), helloCCC_wsgi)
    
    t1 = threading.Thread(target=serverAAA.start) # THERE IS AN ERROR
    t2 = threading.Thread(target=serverBBB.start) # THERE IS AN ERROR
    t3 = threading.Thread(target=serverCCC.start) # THERE IS AN ERROR
    
    t1.start()
    t2.start()
    t3.start()
    
    print('*** all servers have started! *** (we are lying here right now :))')
    
    # XXX   IT IS WRONG!
    #       we can't really say about finishing start-procedure.
    #       and we can't be sure about ability to invoke ``stop()`` (see below).
    #       if ``start()`` hadn't done ready status yet, we couldn't invoke ``stop()``
    
    print('*** serving requests is in process... ***')
    
    signal.sigwaitinfo({signal.SIGINT, signal.SIGTERM})
    
    print('*** terminating... ***')
    
    serverAAA.stop()
    serverBBB.stop()
    serverCCC.stop()
    
    t1.join()
    t2.join()
    t3.join()
    
    # XXX
    #       we won't be here, if any ``stop()`` has invoked too early
    
    print('*** gracefully terminated! ***')

if __name__ == '__main__':
    main()
  • What is the new behavior (if this is a feature change)?

please, see my next another example:

# -*- mode: python; coding: utf-8 -*-

from cheroot.wsgi import Server
import signal
import threading

def helloAAA_wsgi(environ, start_response):
    start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
    
    yield 'Hello world! #AAA\n'.encode()

def helloBBB_wsgi(environ, start_response):
    start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
    
    yield 'Hello world! #BBB\n'.encode()

def helloCCC_wsgi(environ, start_response):
    start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
    
    yield 'Hello world! #CCC\n'.encode()

def main():
    signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGINT, signal.SIGTERM})
    
    serverAAA = Server(('127.0.0.1', 8081), helloAAA_wsgi)
    serverBBB = Server(('127.0.0.1', 8082), helloBBB_wsgi)
    serverCCC = Server(('127.0.0.1', 8083), helloCCC_wsgi)
    
    serverAAA.start(dont_serve=True)
    serverBBB.start(dont_serve=True)
    serverCCC.start(dont_serve=True)
    
    print('*** all servers have started! ***')
    
    t1 = threading.Thread(target=serverAAA.serve)
    t2 = threading.Thread(target=serverBBB.serve)
    t3 = threading.Thread(target=serverCCC.serve)
    
    t1.start()
    t2.start()
    t3.start()
    
    print('*** serving requests is in process... ***')
    
    signal.sigwaitinfo({signal.SIGINT, signal.SIGTERM})
    
    print('*** terminating... ***')
    
    # now it is SAFE to invoke ``stop()`` here
    
    serverAAA.stop()
    serverBBB.stop()
    serverCCC.stop()
    
    t1.join()
    t2.join()
    t3.join()
    
    print('*** gracefully terminated! ***')

if __name__ == '__main__':
    main()
  • Other information:

  • Checklist:

    • I think the code is well written
    • I wrote good commit messages
    • I have squashed related commits together after the changes have been approved
    • Unit tests for the changes exist
    • Integration tests for the changes exist (if applicable)
    • I used the same coding conventions as the rest of the project
    • The new code doesn't generate linter offenses
    • Documentation reflects the changes
    • The PR relates to only one subject with a clear title
      and description in grammatically correct, complete sentences

This change is Reviewable

@codecov

This comment has been minimized.

codecov bot commented May 27, 2018

Codecov Report

Merging #98 into master will increase coverage by 1.38%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
+ Coverage   66.05%   67.44%   +1.38%     
==========================================
  Files          19       19              
  Lines        3067     3781     +714     
==========================================
+ Hits         2026     2550     +524     
- Misses       1041     1231     +190

@webknjaz webknjaz requested review from jaraco and webknjaz May 31, 2018

@jaraco

Without understanding the user's underlying complaint, the change this PR attempts to make seems perfectly reasonable.

I would make one request, though - don't use a boolean parameter to a function to select what that function does. Instead, just call the functions you need. i.e.:

def start(self):
    self.prepare()
    self.serve()

def prepare(self):
    # do the prep

This simpler refactoring provides the necessary hook that prepares the server without starting, but also to start the server, but more clearly and simply.

@polymorphm

This comment has been minimized.

Contributor

polymorphm commented Jul 14, 2018

@jaraco I agree with your code suggestion. (I thought about it early, but at that time I was afraid doing it, cause it is more big change)

I've just changed my branch to your suggestion.

@polymorphm

This comment has been minimized.

Contributor

polymorphm commented Jul 14, 2018

the example of using now is

    # -*- mode: python; coding: utf-8 -*-

    from cheroot.wsgi import Server
    import signal
    import threading

    def helloAAA_wsgi(environ, start_response):
        start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
        
        yield 'Hello world! #AAA\n'.encode()

    def helloBBB_wsgi(environ, start_response):
        start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
        
        yield 'Hello world! #BBB\n'.encode()

    def helloCCC_wsgi(environ, start_response):
        start_response('200 OK', [('Content-type','text/plain;charset=utf-8')])
        
        yield 'Hello world! #CCC\n'.encode()

    def main():
        signal.pthread_sigmask(signal.SIG_BLOCK, {signal.SIGINT, signal.SIGTERM})
        
        serverAAA = Server(('127.0.0.1', 8081), helloAAA_wsgi)
        serverBBB = Server(('127.0.0.1', 8082), helloBBB_wsgi)
        serverCCC = Server(('127.0.0.1', 8083), helloCCC_wsgi)
        
        serverAAA.prepare()
        serverBBB.prepare()
        serverCCC.prepare()
        
        print('*** all servers have started! ***')
        
        t1 = threading.Thread(target=serverAAA.serve)
        t2 = threading.Thread(target=serverBBB.serve)
        t3 = threading.Thread(target=serverCCC.serve)
        
        t1.start()
        t2.start()
        t3.start()
        
        print('*** serving requests is in process... ***')
        
        signal.sigwaitinfo({signal.SIGINT, signal.SIGTERM})
        
        print('*** terminating... ***')
        
        # now it is SAFE to invoke ``stop()`` here
        
        serverAAA.stop()
        serverBBB.stop()
        serverCCC.stop()
        
        t1.join()
        t2.join()
        t3.join()
        
        print('*** gracefully terminated! ***')

    if __name__ == '__main__':
        main()
@webknjaz

LGTM.
@polymorphm would you mind adding tests/docs as well?

Resolved.

@webknjaz webknjaz requested a review from jaraco Jul 16, 2018

@polymorphm

This comment has been minimized.

Contributor

polymorphm commented Jul 28, 2018

@webknjaz > would you mind adding tests/docs as well?

I added two tests, with hoping them are useful :-) .

Is there documentation only in cheroot/cheroot/server.py header doc-string?

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jul 28, 2018

@polymorphm you may create an .rst file under docs/, it should work.

def test_prepare_makes_server_ready():
"""Check that prepare() makes the server ready, and stop() clears it."""
httpserver = HTTPServer(
bind_addr=(ANY_INTERFACE_IPV6, EPHEMERAL_PORT),

This comment has been minimized.

@webknjaz

webknjaz Jul 28, 2018

Member

This might be unstable in case of missing IPv6.

This comment has been minimized.

@polymorphm

polymorphm Jul 28, 2018

Contributor

you're right

@polymorphm

This comment has been minimized.

Contributor

polymorphm commented Jul 29, 2018

I've added some words about starting and stopping a server and made links-to-functions "clickable". It looks ready enough in my humble opinion :-) .

I apologize for delays from my side

@@ -39,6 +39,21 @@
if req.close_connection:
return
For running a server you can invoke :func:`start() <HTTPServer.start()>` (it

This comment has been minimized.

@webknjaz

webknjaz Aug 1, 2018

Member

Have you tried rendering it? I think it's a broken reference.

This comment has been minimized.

@polymorphm

polymorphm Aug 1, 2018

Contributor

I rendered via

pip install '.[docs]' && sphinx-build docs docs/_build

it did this HTML:

<p>For running a server you can invoke <a class="reference internal" href="#cheroot.server.HTTPServer.start" title="cheroot.server.HTTPServer.start"><code class="xref py py-func docutils literal notranslate"><span class="pre">start()</span></code></a> (it
will run the server forever) or use invoking <a class="reference internal" href="#cheroot.server.HTTPServer.prepare" title="cheroot.server.HTTPServer.prepare"><code class="xref py py-func docutils literal notranslate"><span class="pre">prepare()</span></code></a> and <a class="reference internal" href="#cheroot.server.HTTPServer.serve" title="cheroot.server.HTTPServer.serve"><code class="xref py py-func docutils literal notranslate"><span class="pre">serve()</span></code></a> like this:</p>

where the reference is

<dl class="method">
<dt id="cheroot.server.HTTPServer.start">
<code class="descname">start</code><span class="sig-paren">(</span><span class="sig-paren">)</span><a class="headerlink" href="#cheroot.server.HTTPServer.start" title="Permalink to this definition">¶</a></dt>
<dd><p>Run the server forever.</p>
<p>It is shortcut for invoking <a class="reference internal" href="#cheroot.server.HTTPServer.prepare" title="cheroot.server.HTTPServer.prepare"><code class="xref py py-func docutils literal notranslate"><span class="pre">prepare()</span></code></a> then <a class="reference internal" href="#cheroot.server.HTTPServer.serve" title="cheroot.server.HTTPServer.serve"><code class="xref py py-func docutils literal notranslate"><span class="pre">serve()</span></code></a>.</p>
</dd></dl>

It looks correct for me.

may be I should use some other command for rendering? could you write it to me please?

This comment has been minimized.

@webknjaz

webknjaz Aug 1, 2018

Member

No, looks fine. It's just that normally you don't need to put braces there.

@webknjaz webknjaz merged commit b7fea72 into cherrypy:master Aug 1, 2018

8 checks passed

LGTM analysis: Python No alert changes
Details
WIP ready for review
Details
ci/circleci: linux-build Your tests passed on CircleCI!
Details
ci/circleci: macos-build Your tests passed on CircleCI!
Details
codeclimate 3 fixed issues
Details
codecov/patch 100% of diff hit (target 66.05%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 1, 2018

@polymorphm thanks!
Release of v6.4.0 is on its way to PYPI: https://travis-ci.org/cherrypy/cheroot/builds/410767541.
It will be available @ https://pypi.org/p/Cheroot/6.4.0 (ETA 30 min).

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 1, 2018

Testing under osx/3.7 env failed because of missing virtualenv-init there, I've manually triggered deployment job.

cc: @jaraco

@webknjaz

This comment has been minimized.

Member

webknjaz commented Aug 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment