Catch exceptions in password_providers

[NicolaiSoeborg]

Catch exceptions during Initialization of password_providers.

Currently synapse will just fail to start with no error message if a password provider throws an exception in __init__.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[erikjohnston]

Personally I think failing is the right thing to do, in the same way as we fail if we can't pass the config. What do you think about raising a ConfigError (with helpful info) instead?

[erikjohnston]

This would then be something like:

Suggested change

	logger.warn("Error while initializing %r: %s", module, e)
	raise ConfigError("Error while initializing %r: %s" % (module, e))

[clokep]

Will this work though since we're past the configuration stage when this gets instantiated?

[erikjohnston]

That is... true. I'm failing to find an example of us failing while starting up outside of database and config sensibly atm :/

[NicolaiSoeborg]

I've added a raise, so it will block synapse from starting with a bad password_provider, but still have the logger.warn:

Before this PR:

python3 -m synapse.app.homeserver --config-path /etc/matrix-synapse/homeserver.yaml
2020-10-30 20:55:57,218 - user - 317 - WARNING - None - ***** STARTING SERVER *****
2020-10-30 20:55:57,219 - user - 318 - WARNING - None - Server /home/user/synapse/app/homeserver.py version 1.22.0rc1 (b=develop,054a6b953,dirty)

After this PR:

python3 -m synapse.app.homeserver --config-path /etc/matrix-synapse/homeserver.yaml
2020-10-30 20:55:57,218 - user - 317 - WARNING - None - ***** STARTING SERVER *****
2020-10-30 20:55:57,219 - user - 318 - WARNING - None - Server /home/user/synapse/app/homeserver.py version 1.22.0rc1 (b=develop,054a6b953,dirty)
2020-10-30 21:04:05,009 - synapse.handlers.auth - 180 - WARNING - None - Error while initializing <class 'synapse_mfa.SynapseMFA'>: division by zero

I wanted to maybe catch the error higher in the stack, but if I print the stacktrace I just get:

Traceback (most recent call last):
  File "/root/matrix/synapse/synapse/handlers/auth.py", line 176, in __init__
    self.password_providers = [
  File "/root/matrix/synapse/synapse/handlers/auth.py", line 177, in <listcomp>
    module(config=config, account_handler=account_handler)
  File "/root/matrix/synapse-mfa/synapse_mfa.py", line 12, in __init__
    self.a = 0/0  # some error
ZeroDivisionError: division by zero

Not sure what initializes AuthHandler or if something fancy addErrback can be added to catch errors

[erikjohnston]

I think something is swallowing your stderr, which is where stack traces seem to be going. Do you have no_redirect_stdio set in your config?

On my setup it seems like the error is bubbling up to here:

synapse/synapse/app/homeserver.py

Lines 365 to 370 in 49c1eb9

	try:
	hs.setup()
	except IncorrectDatabaseSetup as e:
	quit_with_error(str(e))
	except UpgradeDatabaseException as e:
	quit_with_error("Failed to upgrade database: %s" % (e,))

which does't catch the exception. I think we probably want to add the same handling as:

synapse/synapse/app/_base.py

Lines 296 to 301 in 49c1eb9

	except Exception:
	traceback.print_exc(file=sys.stderr)
	reactor = hs.get_reactor()
	if reactor.running:
	reactor.stop()
	sys.exit(1)

[erikjohnston]

Ideally we'd attempt to handle the errors as per my previous comment, but equally this is a net improvement so I'm happy to land this if you're not in a position to dig further into it.