Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Parse SYNAPSE_ASYNC_IO_REACTOR env variable & log the reactor on startup #14092

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Parse SYNAPSE_ASYNC_IO_REACTOR env variable & log the reactor on startup #14092

merged 2 commits into from
Oct 7, 2022

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Oct 6, 2022

This properly parses the SYNAPSE_ASYNC_IO_REACTOR environement variable similar to what we do with SYNAPSE_USE_FROZEN_DICTS.

It also logs the reactor class currently being used on startup, to help with debugging.

There also was an unecessary try/catch on the reactor installation that I removed.

This is needed for this PR in sytest: matrix-org/sytest#1301

Related:

@DMRobertson
Copy link
Contributor

Related: #8642, #12135.

@@ -348,3 +350,4 @@ def setup_logging(
)
logging.info("Server hostname: %s", config.server.server_name)
logging.info("Instance name: %s", hs.get_instance_name())
logging.info("Twisted reactor: %s", type(reactor).__name__)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that we need to install the asyncio reactor first before we do this? But that should have already happened at import time.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, the reactor gets installed quite early

Comment on lines -48 to -49
except ImportError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what we were hoping to capture in #12135, but we may as well fail loudly. And besides, this is all still experimental.

Copy link
Member

Choose a reason for hiding this comment

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

See #12135 (comment) for why this is in a try-except.

Copy link
Contributor

Choose a reason for hiding this comment

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

CI was fully green on this PR. I guess we had to fixup whatever that comment was talking about as part of the Rust work in #12595 et al?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe! Not sure. 🤷

@sandhose
Copy link
Member Author

sandhose commented Oct 7, 2022

@DMRobertson I don't have merge rights on Synapse, so if you're fine with this change, could you merge it for me?

@DMRobertson
Copy link
Contributor

Sorry, will do.

@DMRobertson DMRobertson merged commit dc37b68 into matrix-org:develop Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants