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

Display the actual error when a pushkin cannot be created. #125

Merged
merged 1 commit into from
May 13, 2020

Conversation

clokep
Copy link
Member

@clokep clokep commented May 13, 2020

Error handling is hard. 😢

This is a follow-up to #122 to show the true error if a pushkin cannot be created, e.g. I added the following to my config:

apps:
  foobar:
    type: apns

And you get a nice(r) error:

2020-05-13 08:11:19,573 [69312] INFO  __main__ Using sqlite database
2020-05-13 08:11:19,582 [69312] INFO  __main__ Importing pushkin module: sygnal.apnspushkin
2020-05-13 08:11:19,612 [69312] INFO  __main__ Creating pushkin: ApnsPushkin
2020-05-13 08:11:19,612 [69312] ERROR __main__ Failed to load and create pushkin for kind 'apns'
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 "/matrix/sygnal/sygnal/sygnal.py", line 222, 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 "/matrix/sygnal/sygnal/sygnal.py", line 222, 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 "/matrix/sygnal/sygnal/sygnal.py", line 191, in _make_pushkins_then_start
    self.pushkins[app_id] = await self._make_pushkin(app_id, app_cfg)
  File "/matrix/sygnal/sygnal/sygnal.py", line 186, in _make_pushkin
    return await clarse.create(app_name, self, app_config)
  File "/matrix/sygnal/sygnal/notifications.py", line 129, in create
    return cls(name, sygnal, config)
  File "/matrix/sygnal/sygnal/apnspushkin.py", line 111, in __init__
    "You must provide a path to an APNs certificate, or an APNs token."
sygnal.exceptions.PushkinSetupException: You must provide a path to an APNs certificate, or an APNs token.

This will help debug #124.

@clokep clokep requested a review from a team May 13, 2020 12:19
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.

Should we do a 0.6.1 with this fix?

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

are we not going to have similar problems with the other places that #122 changed?

(presumably when we raise the RuntimeError, the old exception is set as a __cause__ on the new, so an alternative approach might be to have https://github.com/matrix-org/sygnal/pull/122/files#diff-13c51378d37f89582bc66a30f28f723bR226 log the cause, somehow, which might give a slightly clearer output? not sure though)

@clokep
Copy link
Member Author

clokep commented May 13, 2020

are we not going to have similar problems with the other places that #122 changed?

The other places in #122 are brand new exceptions, nothing is caught in those cases, so I don't think there's anything "better" to log in those cases.

@richvdh
Copy link
Member

richvdh commented May 13, 2020

The other places in #122 are brand new exceptions, nothing is caught in those cases, so I don't think there's anything "better" to log in those cases.

good point!

@clokep
Copy link
Member Author

clokep commented May 13, 2020

I did debate using a custom exception (StartError) and handling that separately to just print the string instead of the whole stack trace. Do you think that would make anything clearer @richvdh?

@richvdh
Copy link
Member

richvdh commented May 13, 2020

Do you think that would make anything clearer @richvdh?

probably not worth it. suggest you go with what you've got.

@clokep clokep merged commit b9d5c51 into master May 13, 2020
@clokep clokep deleted the clokep/better-exc-logging branch May 13, 2020 14:17
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.

3 participants