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

Log errors during start-up and fix the default logging config #122

Merged
merged 2 commits into from
May 12, 2020

Conversation

clokep
Copy link
Member

@clokep clokep commented May 12, 2020

This copies (a slimmed down) version of the code we use for Synapse to log errors during start-up.

With this I'm able to get a sane error message if I have a bad config:

$ python -m sygnal.sygnal
The following configuration fields are not understood: {'database'}
Error during startup:
Traceback (most recent call last):
  File "/.virtualenvs/sygnal/lib/python3.7/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
    result = g.send(result)
  File "/sygnal/sygnal/sygnal.py", line 218, in start
    port, bind_addresses, pushgateway_api
  File "/.virtualenvs/sygnal/lib/python3.7/site-packages/twisted/internet/defer.py", line 911, in ensureDeferred
    return _cancellableInlineCallbacks(coro)
  File "/.virtualenvs/sygnal/lib/python3.7/site-packages/twisted/internet/defer.py", line 1529, in _cancellableInlineCallbacks
    _inlineCallbacks(None, g, status)
--- <exception caught here> ---
  File "/sygnal/sygnal/sygnal.py", line 218, in start
    port, bind_addresses, pushgateway_api
  File "/.virtualenvs/sygnal/lib/python3.7/site-packages/twisted/internet/defer.py", line 1418, in _inlineCallbacks
    result = g.send(result)
  File "/sygnal/sygnal/sygnal.py", line 196, in _make_pushkins_then_start
    "No app IDs are configured. Edit sygnal.yaml to define some."
builtins.RuntimeError: No app IDs are configured. Edit sygnal.yaml to define some.

Fixes #121
Fixes #120

@@ -210,9 +210,25 @@ def run(self):
bind_addresses = self.config["http"]["bind_addresses"]
pushgateway_api = PushGatewayApiServer(self)

ensureDeferred(
Copy link
Member Author

Choose a reason for hiding this comment

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

This in particular had me scratch my head since...it makes a Deferred but never does anything with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc that schedules that coroutine on the Reactor (if I remember the Twisted words correctly).

@clokep clokep requested a review from a team May 12, 2020 12:44
@clokep
Copy link
Member Author

clokep commented May 12, 2020

One thing to note with this is that the process does exit with a success code (0). Adding a sys.exit(1) caused an additional exception to be raised, but still exited as success.

babolivier
babolivier previously approved these changes May 12, 2020
Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

sygnal/sygnal.py Show resolved Hide resolved
sygnal/sygnal.py Show resolved Hide resolved
@clokep
Copy link
Member Author

clokep commented May 12, 2020

I also added the fix for #120 to this.

@clokep clokep requested a review from babolivier May 12, 2020 14:26
@clokep clokep changed the title Log errors during start-up. Log errors during start-up and fix the default logging config May 12, 2020
@clokep clokep merged commit 1e23bd4 into master May 12, 2020
@clokep clokep deleted the clokep/startup-logging branch May 12, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sys.exit(1) doesn't actually stop Sygnal The sample logging configuration doesn't work
3 participants