Wrap the Rust HTTP client with `make_deferred_yieldable` #18903

MadLittleMods · 2025-09-09T03:58:28Z

Wrap the Rust HTTP client with make_deferred_yieldable so downstream usage doesn't need to use PreserveLoggingContext() or make_deferred_yieldable.

it seems like we should have some wrapper around it that uses make_deferred_yieldable(...) to make things right so we don't have to do this in the downstream code.

-- @MadLittleMods, #18357 (comment)

Spawning from wanting to remove PreserveLoggingContext() from the codebase and thinking that we shouldn't have to pollute all downstream usage with PreserveLoggingContext() or make_deferred_yieldable

Part of #18905 (Remove sentinel logcontext where we log in Synapse)

Dev notes

Tokio runtime is not running -> #18903 (comment)

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.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct (run the linters)

So downstream usage doesn't need to use `PreserveLoggingContext()` or `make_deferred_yieldable` Spawning from #18870 and #18357 (comment)

synapse/synapse_rust_wrapper/http_client.py

reivilibre

OK to merge or otherwise would be happy to see the wrapperless approach if you have the energy for it

synapse/synapse_rust_wrapper/http_client.py

…ve-preserve-logging-context

tests/synapse_rust/test_http_client.py

…ve-preserve-logging-context

See #18903 (comment) --- Related to twisted/twisted#12514

See #18903 (comment)

MadLittleMods · 2025-09-30T22:26:51Z

rust/src/http_client.rs

+/// Given a deferred, make it follow the Synapse logcontext rules
+fn make_deferred_yieldable<'py>(
+    py: Python<'py>,
+    deferred: &Bound<'py, PyAny>,
+) -> Bound<'py, PyAny> {
+    let make_deferred_yieldable = MAKE_DEFERRED_YIELDABLE.get_or_init(|| {
+        let sys = PyModule::import(py, "synapse.logging.context").unwrap();
+        let func = sys.getattr("make_deferred_yieldable").unwrap().unbind();
+        func
+    });
+
+    make_deferred_yieldable
+        .call1(py, (deferred,))
+        .unwrap()
+        .extract(py)
+        .unwrap()
+}


This is based on @reivilibre suggestion in #18903 (comment)

But please double-check that I didn't fumble the lifetimes, etc trying to kludge things together.

MadLittleMods · 2025-09-30T22:33:37Z

tests/synapse_rust/test_http_client.py

+                        url=self.server.endpoint,
+                        response_limit=1 * 1024 * 1024,
+                    )
+                    self._check_current_logcontext("competing")


I've confirmed this test fails without the changes to rust/src/http_client.rs

To reproduce:

Checkout this PR: git checkout madlittlemods/18357-rust-http-client-remove-preserve-logging-context

Use the old version of the Rust HTTP Client: git checkout develop -- rust/src/http_client.rs

Rebuild Rust: poetry install --extras all

Run the test: SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.synapse_rust.test_http_client.HttpClientTestCase.test_logging_context

SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.synapse_rust.test_http_client.HttpClientTestCase.test_logging_context tests.synapse_rust.test_http_client HttpClientTestCase test_logging_context ... [ERROR] =============================================================================== [ERROR] Traceback (most recent call last): File "/virtualenvs/matrix-synapse-xCtC9ulO-py3.13/lib/python3.13/site-packages/twisted/internet/defer.py", line 1857, in _inlineCallbacks result = context.run(gen.send, result) File "synapse/tests/synapse_rust/test_http_client.py", line 196, in do_request self._check_current_logcontext("competing") File "synapse/tests/synapse_rust/test_http_client.py", line 151, in _check_current_logcontext self.assertEqual( File "/virtualenvs/matrix-synapse-xCtC9ulO-py3.13/lib/python3.13/site-packages/twisted/trial/_synctest.py", line 444, in assertEqual super().assertEqual(first, second, msg) File "/usr/lib/python3.13/unittest/case.py", line 907, in assertEqual assertion_func(first, second, msg=msg) File "/usr/lib/python3.13/unittest/case.py", line 1273, in assertMultiLineEqual self.fail(self._formatMessage(msg, standardMsg)) twisted.trial.unittest.FailTest: 'sentinel' != 'competing' - sentinel + competing : Expected LoggingContext(competing) but saw sentinel tests.synapse_rust.test_http_client.HttpClientTestCase.test_logging_context ------------------------------------------------------------------------------- Ran 1 tests in 0.251s FAILED (errors=1)

MadLittleMods · 2025-09-30T22:34:24Z

tests/synapse_rust/test_http_client.py

+        self.get_success(self.till_deferred_has_result(do_request()))
+        self.assertEqual(self.server.calls, 1)
+
+    async def test_logging_context(self) -> None:


This test is based off the tests we have in tests/util/test_logcontext.py

MadLittleMods · 2025-09-30T22:36:56Z

synapse/synapse_rust/http_client.pyi

+    """
+    The returned deferreds follow Synapse logcontext rules.
+    """


Perhaps we should just remove this as we should just assume that everything in Synapse does follow the Synapse logcontext rules unless otherwise stated.

For example:

synapse/synapse/logging/context.py

Lines 853 to 856 in d1c96ee

Returns:

Deferred which returns the result of func, or `None` if func raises.

Note that the returned Deferred does not follow the synapse logcontext

rules.

…ve-preserve-logging-context

MadLittleMods · 2025-09-30T22:43:12Z

tests/synapse_rust/test_http_client.py

+        # XXX: We must create the Rust HTTP client before we call `reactor.run()` below.
+        # Twisted's `MemoryReactor` doesn't invoke `callWhenRunning` callbacks if it's
+        # already running and we rely on that to start the Tokio thread pool in Rust. In
+        # the future, this may not matter, see https://github.com/twisted/twisted/pull/12514


Created a Twisted PR to fix this flaw in the MemoryReactor -> twisted/twisted#12514

Seems to have buy-in (approved) so hopefully we don't need to deal with this in the future.

MadLittleMods added 3 commits September 8, 2025 22:56

Wrap the Rust HTTP client with make_deferred_yieldable

986e15c

So downstream usage doesn't need to use `PreserveLoggingContext()` or `make_deferred_yieldable` Spawning from #18870 and #18357 (comment)

Add chagnelog

5e555f3

Docstring for why we have a wrapper

4e0085c

MadLittleMods commented Sep 9, 2025

View reviewed changes

synapse/synapse_rust_wrapper/http_client.py Outdated Show resolved Hide resolved

MadLittleMods marked this pull request as ready for review September 9, 2025 04:04

MadLittleMods requested a review from a team as a code owner September 9, 2025 04:04

This was referenced Sep 9, 2025

Speed up MAS token introspection #18357

Merged

Remove sentinel logcontext where we log in setup, start and exit #18870

Merged

reivilibre approved these changes Sep 9, 2025

View reviewed changes

synapse/synapse_rust_wrapper/http_client.py Outdated Show resolved Hide resolved

Merge branch 'develop' into madlittlemods/18357-rust-http-client-remo…

763cdb9

…ve-preserve-logging-context

MadLittleMods mentioned this pull request Sep 9, 2025

replace loggingcontexts with contextvars #10342

Open

WIP: Add test

ce6f16d

MadLittleMods commented Sep 9, 2025

View reviewed changes

tests/synapse_rust/test_http_client.py Show resolved Hide resolved

This was referenced Sep 10, 2025

Fix server_name in logging context for multiple Synapse instances in one process #18868

Merged

Remove sentinel logcontext where we log in Synapse #18905

Open

MadLittleMods added the A-Logging label Sep 10, 2025

MadLittleMods added 4 commits September 17, 2025 09:59

Merge branch 'develop' into madlittlemods/18357-rust-http-client-remo…

d4558df

…ve-preserve-logging-context

Merge branch 'develop' into madlittlemods/18357-rust-http-client-remo…

dce9754

…ve-preserve-logging-context

Merge branch 'develop' into madlittlemods/18357-rust-http-client-remo…

9def6f7

…ve-preserve-logging-context

Maintenance from merging in the latest develop changes

77970eb

MadLittleMods mentioned this pull request Sep 30, 2025

Fix MemoryReactor.callWhenRunning not invoking callbacks if already started twisted/twisted#12514

Open

MadLittleMods added 9 commits September 30, 2025 15:38

Fix Tokio runtime is not running

b89fb2e

See #18903 (comment) --- Related to twisted/twisted#12514

Add working test

34b6f2a

Add basic request/response test

8a9d682

Add docstring

2d341cb

Better test that fails

58c1209

Cross-link Twisted PR

9a4e67d

WIP: Failing test

59516a4

Use make_deferred_yieldable in Rust

74f01a9

See #18903 (comment)

Clean up test

4424a88

MadLittleMods added 2 commits September 30, 2025 17:24

Remove Rust wrapper (no longer necessary)

e6df6ee

Note that the returned deferred follow Synapse logcontext rules now

92ea10a

MadLittleMods commented Sep 30, 2025

View reviewed changes

MadLittleMods added 2 commits September 30, 2025 17:27

No more Rust wrapper (switch back to Rust imports)

5c324bc

Fix lints

e243b0f

MadLittleMods commented Sep 30, 2025

View reviewed changes

MadLittleMods added 2 commits September 30, 2025 17:39

Move imports (try fix lints)

ab453b7

Merge branch 'develop' into madlittlemods/18357-rust-http-client-remo…

a0051f7

…ve-preserve-logging-context

MadLittleMods commented Sep 30, 2025

View reviewed changes

MadLittleMods requested a review from reivilibre September 30, 2025 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Wrap the Rust HTTP client with `make_deferred_yieldable` #18903

Wrap the Rust HTTP client with `make_deferred_yieldable` #18903

MadLittleMods commented Sep 9, 2025 •

edited

Loading

Uh oh!

Uh oh!

reivilibre left a comment

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Sep 30, 2025

Uh oh!

MadLittleMods Sep 30, 2025

Uh oh!

MadLittleMods Sep 30, 2025

Uh oh!

MadLittleMods Sep 30, 2025

Uh oh!

MadLittleMods Sep 30, 2025

Uh oh!

Uh oh!

	Returns:
	Deferred which returns the result of func, or `None` if func raises.
	Note that the returned Deferred does not follow the synapse logcontext
	rules.

Wrap the Rust HTTP client with make_deferred_yieldable #18903

Are you sure you want to change the base?

Wrap the Rust HTTP client with make_deferred_yieldable #18903

Conversation

MadLittleMods commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Pull Request Checklist

Uh oh!

Uh oh!

reivilibre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MadLittleMods Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

MadLittleMods Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wrap the Rust HTTP client with `make_deferred_yieldable` #18903

Wrap the Rust HTTP client with `make_deferred_yieldable` #18903

MadLittleMods commented Sep 9, 2025 •

edited

Loading