mirror of
https://github.com/element-hq/synapse.git
synced 2026-07-11 20:39:22 +00:00
Eric Eastwood
c63d77a79d
This is a stepping stone before we can go full Rust everywhere. We're providing a generic interface as we want database access to work in Synapse and `synapse-rust-apps`. In `synapse-rust-apps`, we will use a `tokio-postgres` based database connection pool so it's full Rust. We want to avoid the situation where we have two database connection pools (one for Python, one for Rust) as we've run into connection exhaustion problems on Matrix.org before. As an example of using it and sanity check for all this work (including tests), I've also ported over the `/versions` handler to the Rust side with database access. The `/versions` endpoint is the simplest endpoint I could find that still had some database access. Hopefully the refactor on `/versions` isn't that controversial as it's not really the point of this PR. We can always remove it from this PR but it's just here as a sanity check that all of this works. ### Why `runInteraction(...)`? Using the same `runInteraction` pattern that we already have in Synapse means we can port over existing Synapse code/endpoints without much thought. But this pattern also makes sense because we want[^1] transactions to have repeatable-read isolation (easy to think about, less foot-guns). Having everything thappen in a function callback means we can do retries for serialization/deadlock errors. [^1]: To note: Ideally, we'd want the least isolation possible but the problem is that there is no tooling to yell at you when your queries/logic is wrong so repeatable-read isolation is a great balance. > When an application receives this error message, it should abort the current transaction and retry the whole transaction from the beginning. The second time through, the transaction will see the previously-committed change as part of its initial view of the database, so there is no logical conflict in using the new version of the row as the starting point for the new transaction's update. > > Note that only updating transactions might need to be retried; read-only transactions will never have serialization conflicts. > > *-- https://www.postgresql.org/docs/current/transaction-iso.html#XACT-REPEATABLE-READ* As a note, this strategy is less of an impedance mismatch (aligns more closely) with Synapse so the glue code for the `python_db_pool` should also be simpler. ### How does this interact with logcontext (`LoggingContext`)? See [docs on log contexts](https://github.com/element-hq/synapse/blob/4e9f7757f17ba81b8747b7f8f9646d17df145aa3/docs/log_contexts.md) for more background. We already support normal logging from Rust -> Python with `pyo3-log` and `log` but as soon as we pass a thread boundary, everything is logged against the `sentinel` log context. Normally, we want logs and CPU/DB usage correlated with the request that spawned the work. You can see how I took a stab at fixing this in https://github.com/element-hq/synapse/pull/19846 by capturing the logcontext in a Tokio task local and re-activating as necessary. For example, in that PR, I reactivated the logcontext in `run_python_awaitable(...)` which we use to call `runInteraction(...)` from the Rust side which means all of the database usage is correlated with the request as expected. It also means any `log:info!(...)` done in `run_interaction(...)` is correlated correctly. But there needs to be a better story for when you want to log everywhere else. I haven't explored tracking CPU usage on the Rust side. I've left all of this out of this PR as I think it will be better to tackle this as a dedicated follow-up. For example, I'm thinking about instead creating a new `LoggingContext` with the `parent_context` set to the calling context and try to avoid needing to call `set_current_context(...)` on the Python side where possible (like tracking CPU). ### Testing strategy Added some tests that exercise some `async` Rust handlers for the `/versions` endpoint: ``` SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.rest.client.test_versions.VersionsTestCase ``` Real-world: 1. `poetry run synapse_homeserver --config-path homeserver.yaml` 1. `GET http://localhost:8008/_matrix/client/versions`