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

Allow timeout of 0 for WSGIServer #1528

Merged
merged 1 commit into from
Jan 10, 2017
Merged

Allow timeout of 0 for WSGIServer #1528

merged 1 commit into from
Jan 10, 2017

Conversation

Safihre
Copy link
Contributor

@Safihre Safihre commented Dec 5, 2016

No description provided.

@Safihre
Copy link
Contributor Author

Safihre commented Dec 5, 2016

I don't think the test-fail has anything to do with this PR, does it?

@@ -1695,7 +1695,7 @@ def stop(self, timeout=5):

# Don't join currentThread (when stop is called inside a request).
current = threading.currentThread()
if timeout and timeout >= 0:
if timeout is not None and timeout >= 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please extend existing tests with corresponding test cases for negative/positive testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oke, in which file would that be? I opened all the ones related to wsgiserver but didn't see things related to startup/shutdown.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. It's needed to check tests. Make your suggestion. You may even need to add some new test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't understand the existing tests sets and where to even begin adding this. I want to help, but this is a bit too much..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll try looking into this myself :)

Safihre added a commit to sabnzbd/sabnzbd that referenced this pull request Dec 5, 2016
Safihre added a commit to sabnzbd/sabnzbd that referenced this pull request Dec 8, 2016
@webknjaz webknjaz added the bug label Dec 17, 2016
@Safihre
Copy link
Contributor Author

Safihre commented Jan 6, 2017

@jaraco @webknjaz If you can give me a hint where I should add the test?
I can't find the startup/shutdown/restart tests.

@jaraco
Copy link
Member

jaraco commented Jan 7, 2017

I don't think there are yet such tests. Given that, I'm inclined to accept this change without tests. If the CherryPy maintainers aren't willing to develop the tests for basic functionality, I don't think we should impose that burden on contributors willing to make minor improvements.

@Safihre Can you at least provide a transcript demonstrating the use of this new feature and that it works?

@Safihre
Copy link
Contributor Author

Safihre commented Jan 9, 2017

@jaraco Of course:
Setting:

    cherrypy.config.update({
                            'server.shutdown_timeout': 0,
                            })

Without the change the assignment of endtime is not made and it crashes:

2017-01-09 09:03:17,382::INFO::[_cplogging:219] [09/Jan/2017:09:03:17] ENGINE Bus STOPPING
2017-01-09 09:03:17,490::INFO::[_cplogging:219] [09/Jan/2017:09:03:17] ENGINE HTTP Server cherrypy._cpwsgi_server.CPWSGIServer(('127.0.0.1', 8080)) shut down
2017-01-09 09:03:17,502::ERROR::[_cplogging:219] [09/Jan/2017:09:03:17] ENGINE Error in 'stop' listener <bound method Server.stop of <cherrypy._cpserver.Server object at 0x03513970>>
Traceback (most recent call last):
  File "C:\Users\<USERNAME>\Documents\GitHub\sabnzbd\cherrypy\process\wspbus.py", line 207, in publish
    output.append(listener(*args, **kwargs))
  File "C:\Users\<USERNAME>\Documents\GitHub\sabnzbd\cherrypy\process\servers.py", line 243, in stop
    self.httpserver.stop()
  File "C:\Users\<USERNAME>\Documents\GitHub\sabnzbd\cherrypy\wsgiserver\__init__.py", line 2222, in stop
    self.requests.stop(self.shutdown_timeout)
  File "C:\Users\<USERNAME>\Documents\GitHub\sabnzbd\cherrypy\wsgiserver\__init__.py", line 1710, in stop
    remaining_time = endtime - time.time()
UnboundLocalError: local variable 'endtime' referenced before assignment

With this change correct and instant shutdown:

2017-01-09 09:00:31,226::INFO::[_cplogging:219] [09/Jan/2017:09:00:31] ENGINE Bus STOPPING
2017-01-09 09:00:31,994::INFO::[_cplogging:219] [09/Jan/2017:09:00:31] ENGINE HTTP Server cherrypy._cpwsgi_server.CPWSGIServer(('127.0.0.1', 8080)) shut down
2017-01-09 09:00:31,997::INFO::[_cplogging:219] [09/Jan/2017:09:00:31] ENGINE Stopped thread '_TimeoutMonitor'.
2017-01-09 09:00:32,000::INFO::[_cplogging:219] [09/Jan/2017:09:00:32] ENGINE Bus STOPPED
2017-01-09 09:00:32,003::INFO::[_cplogging:219] [09/Jan/2017:09:00:32] ENGINE Bus EXITING
2017-01-09 09:00:32,006::INFO::[_cplogging:219] [09/Jan/2017:09:00:32] ENGINE Bus EXITED

@jaraco jaraco merged commit e1abe60 into cherrypy:master Jan 10, 2017
jaraco added a commit to cherrypy/cheroot that referenced this pull request Jan 10, 2017
jaraco added a commit that referenced this pull request Jan 10, 2017
jaraco added a commit that referenced this pull request Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants