progrium/ginkgo

avoid printing exceptions (traceback.print_exc)

progrium opened this issue · 4 comments

In Service.start() and .serve_forever() we catch all exceptions and call stop(). However, we added traceback.print_exc() so that if stop() raises, we can see the original exception. It seems like there is a better way to do this. Perhaps an inner try/except? Even though tests pass, we still get nasty, confusing output:

progrium-twilio:gservice progrium$ python setup.py test
running test
............................Traceback (most recent call last):
  File "/Users/progrium/Projects/gservice/gservice/core.py", line 179, in start
    ready = self.do_start()
  File "/Users/progrium/Projects/gservice/gservice/tests/test_service.py", line 49, in do_start
    raise Exception("Error")
Exception: Error
.Traceback (most recent call last):
  File "/Users/progrium/Projects/gservice/gservice/core.py", line 181, in start
    self._ready_event.wait(self.ready_timeout)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/gevent-0.13.3-py2.7-macosx-10.6-x86_64.egg/gevent/event.py", line 74, in wait
    result = get_hub().switch()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/gevent-0.13.3-py2.7-macosx-10.6-x86_64.egg/gevent/hub.py", line 163, in switch
    return greenlet.switch(self)
NotImplementedError: Second Error
...............
----------------------------------------------------------------------
Ran 44 tests in 7.403s

OK

We should consider logging the exception instead of printing it. It will make debugging and testing easier.

Alan suggests we have two options: swallowing it or aborting (just die). Service.stop() should not raise. If it did, it's the Service implementor's fault.

Okay, we know that Service.stop() should not call Service.do_stop() if the service is not started, but should call stop() on its children.

We also decided to remove the try...except inside start() (and serve_forever()) that will try to call stop() on an exception.

I think the latest master satisfies this. I remember this was a nebulous issue that got into a bunch of other issues... most of which seem to have been resolved after 0.5.0