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

Allow enabling the asyncio reactor in complement #14099

Closed
wants to merge 4 commits into from
Closed

Allow enabling the asyncio reactor in complement #14099

wants to merge 4 commits into from

Conversation

sandhose
Copy link
Member

@sandhose sandhose commented Oct 7, 2022

This allows enabling the asyncio reactor by setting the ASYNCIO_REACTOR=1 environment variable.
It also adds jobs to the CI to run those in CI.

@reivilibre
Copy link
Contributor

Concerned that having 3 more Complement runs per CI job is going to make our problem of running out of CI workers even worse. But I understand the intention and we don't like running things that aren't tested in CI.
Asked in #synapse-dev for more opinions on the matter.

What's your intention here? Are you going to be relying on the asyncio reactor soon due to a code change?

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

We discussed in #synapse-dev and decided that we'd probably prefer to make this a nightly, or a develop-only, run.
We also note that Erik has been running the asyncio reactor on his homeserver and it hasn't seemed to explode. I will add to that and say I have been running my homeserver with the asyncio reactor + uvloop's asyncio event loop since Monday and have not had any trouble yet.

@reivilibre
Copy link
Contributor

For a develop-only CI run, you might be interested in waiting for https://github.com/matrix-org/synapse/pull/14153/files#diff-98ea8896abc68eceb35d39a84a9b43975323a3552b8be404cc53fcbce94eb38aR138 — that would give you the ability to write some Python akin to if not IS_PR: and extend the set of Complement jobs that way, rather than e.g. duplicating the GHA YAML for the Complement run.

@DMRobertson
Copy link
Contributor

As noted here, the forking launcher fails with this change.

  2022-10-07 09:56:45,026 INFO spawned: 'synapse_fork' with pid 56
  Traceback (most recent call last):
    File "/usr/local/lib/python3.9/runpy.py", line 197, in _run_module_as_main
      return _run_code(code, main_globals, None,
    File "/usr/local/lib/python3.9/runpy.py", line 87, in _run_code
      exec(code, run_globals)
    File "/usr/local/lib/python3.9/site-packages/synapse/app/complement_fork_starter.py", line 218, in <module>
      main()
    File "/usr/local/lib/python3.9/site-packages/synapse/app/complement_fork_starter.py", line 152, in main
      installReactor(proxy_reactor)
    File "/usr/local/lib/python3.9/site-packages/twisted/internet/main.py", line 32, in installReactor
      raise error.ReactorAlreadyInstalledError("reactor already installed")
  twisted.internet.error.ReactorAlreadyInstalledError: reactor already installed
  2022-10-07 09:56:45,608 INFO exited: synapse_fork (exit status 1; not expected)

Looks like we need to install the asyncio reactor here

def _worker_entrypoint(
func: Callable[[], None], proxy_reactor: ProxiedReactor, args: List[str]
) -> None:
"""
Entrypoint for a forked worker process.
We just need to set up the command-line arguments, create our real reactor
and then kick off the worker's main() function.
"""
sys.argv = args
# reset the custom signal handlers that we installed, so that the children start
# from a clean slate.
for sig, handler in _original_signal_handlers.items():
signal.signal(sig, handler)
from twisted.internet.epollreactor import EPollReactor
proxy_reactor._install_real_reactor(EPollReactor())
func()

and somehow not install the asyncioreactor here??

# Allow using the asyncio reactor via env var.
if strtobool(os.environ.get("SYNAPSE_ASYNC_IO_REACTOR", "0")):
from incremental import Version
import twisted
# We need a bugfix that is included in Twisted 21.2.0:
# https://twistedmatrix.com/trac/ticket/9787
if twisted.version < Version("Twisted", 21, 2, 0):
print("Using asyncio reactor requires Twisted>=21.2.0")
sys.exit(1)
import asyncio
from twisted.internet import asyncioreactor
asyncioreactor.install(asyncio.get_event_loop())

@sandhose
Copy link
Member Author

@DMRobertson I think I found a proper solution: 1c71e6d ; let's see if CI passes.

It installs the asyncioreactor before loading the rest, and unsets the environment variable, so __init__ doesn't try to reinstall the reactor

@DMRobertson
Copy link
Contributor

@DMRobertson I think I found a proper solution: 1c71e6d ; let's see if CI passes.

It installs the asyncioreactor before loading the rest, and unsets the environment variable, so __init__ doesn't try to reinstall the reactor

This didn't work out. To see the problem:

$ SYNAPSE_ASYNC_IO_REACTOR=1 python -v -m synapse.app.complement_fork_starter aa 2>&1 | grep -E 'import.*(synapse|reactor)'
import 'synapse.logging._remote' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c68574d60>
import 'synapse.logging._terse_json' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c68851d50>
import 'synapse.logging' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c68574910>
import 'synapse.logging.context' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c675fca90>
import 'synapse.util' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c695eb1c0>
import 'synapse.synapse_rust' # <_frozen_importlib_external.ExtensionFileLoader object at 0x7f4c68851e10>
import 'synapse.util.rust' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c66b359f0>
import 'synapse.api' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c66b1d720>
import 'synapse.api.errors' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c66b1da80>
import 'synapse.util.stringutils' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c675e6860>
import 'twisted.internet.asyncioreactor' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c66bee2f0>
import 'synapse' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c695e9570>
import 'synapse.util.check_dependencies' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c66782950>
import 'synapse.app' # <_frozen_importlib_external.SourceFileLoader object at 0x7f4c66713a90>

I don't know how the import order is determined, but Python is importing the top-level synapse` module (hence installing the asyncio reactor) before it imports the complement_fork_starter.

Perhaps you could pass a --asyncio flag to the complement_fork_starter specifically?

@sandhose
Copy link
Member Author

Superseded by #14858

@sandhose sandhose closed this Jan 17, 2023
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