Refactor app entrypoints (avoid exit(1) in our composable functions)
#19121
Conversation
Move `register_start` out of `setup` Align all entrypoints (main, worker, `admin_cmd`)
|
|
||
|
|
||
| def start(config: HomeServerConfig, args: argparse.Namespace) -> None: | ||
| def create_homeserver( |
There was a problem hiding this comment.
Refactored all of our entrypoints to use the create_homeserver(...) -> setup(...) -> start(...) pattern
| homeserver_config = load_config(sys.argv[1:]) | ||
| with LoggingContext(name="main", server_name=homeserver_config.server.server_name): | ||
| start(homeserver_config) | ||
| worker_main() |
There was a problem hiding this comment.
Equivalent and simpler 😌
synapse/app/generic_worker.py
Outdated
| # Register a callback to be invoked once the reactor is running | ||
| register_start(hs, lambda: start(hs)) |
There was a problem hiding this comment.
The main fix is moving this register_start(...) from setup(...) (one of our composable functions used in Synapse Pro for small hosts) to here in main(...)
| # everything with this homeserver. | ||
| with LoggingContext(name="main", server_name=homeserver_config.server.server_name): | ||
| # check base requirements | ||
| check_requirements() |
There was a problem hiding this comment.
Removed check_requirements() here as we call it globally in synapse/app/__init__.py#L29-L35
If I add some debug logging to it, we actually it three times when I start Synapse (even after this change):
$ poetry run synapse_homeserver --config-path homeserver.yaml
check_requirements(None)
check_requirements("oidc")
check_requirements("opentracing)
…-hq/synapse into madlittlemods/refactor-app-start
anoadragon453
left a comment
anoadragon453
left a comment
There was a problem hiding this comment.
LGTM. A minor, non-blocking points below.
synapse/app/generic_worker.py
Outdated
| setup(hs) | ||
|
|
||
| # Register a callback to be invoked once the reactor is running | ||
| register_start(hs, lambda: start(hs)) |
There was a problem hiding this comment.
Why lambda: start(hs) instead of:
register_start(hs, start, hs)There was a problem hiding this comment.
🤷 Hard decisions on what's more clear
I've updated register_start(hs, start, hs) even though it can take a second to recognize what's happening
Conflicts: synapse/app/generic_worker.py
|
Thanks for the review @anoadragon453 🐝 (in time for the RC 🚀) |
This regressed in #19121. I moved things in #19121 because I thought that it made sense to redirect anything printed to `stdout`/`stderr` to the logs as early as possible. But we actually want to log any immediately apparent problems during initialization to `stderr` in the terminal so that they are obvious and visible to the operator. Now, I've moved `redirect_stdio_to_logs()` back to where it was previously along with some proper comment context for why we have it there.
Refactor app entrypoints (avoid
exit(1)in our composable functions)register_start(callsos._exit(1)) out ofsetup(our composable function)exit(...)because we use these composable functions in Synapse Pro for small hosts where we have multiple Synapse instances running in the same process. We don't want a problem from one homeserver tenant causing the entire Python process to exit and affect all of the other homeserver tenants.exit(1)in our composable functions) #19116homeserver(main),generic_worker(worker), andadmin_cmdBackground
As part of Element's plan to support a light form of vhosting (virtual host) (multiple instances of Synapse in the same Python process) (c.f Synapse Pro for small hosts), we're currently diving into the details and implications of running multiple instances of Synapse in the same Python process.
"Clean tenant provisioning" tracked internally by https://github.com/element-hq/synapse-small-hosts/issues/48
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.