Replace manual LoggingContext usage with `ModuleApi.defer_to_threadpool` #134

anoadragon453 · 2025-10-01T16:02:28Z

Attempt to replace manual usage of LoggingContext with the provided module API's run_in_background method.

I'm not entirely convinced about the changes to fetch (and subsequently s3_download_task). The fact we hand it a deferred directly is confusing me.

Spawning from #133. #74 can be closed after this PR is merged.

anoadragon453 · 2025-10-01T16:03:24Z

s3_storage_provider.py



-def s3_download_task(s3_client, bucket, key, extra_args, deferred, parent_logcontext):
+def s3_download_task(s3_client, bucket, key, extra_args, deferred):


The only real changes here are:

Remove parent_logcontext.

Removing with LoggingContext ... and de-indenting all of the code underneath it.

I think these changes make sense. s3_download_task will use whatever the caller logcontext is.

And I think we maintain the logcontext when calling s3_download_task ✅ (at-least with the suggested patterns).

Attempt to replace manual usage of LoggingContext with the provided module API's `run_in_background` method.

s3_storage_provider.py

MadLittleMods · 2025-10-01T16:25:06Z

s3_storage_provider.py



-def s3_download_task(s3_client, bucket, key, extra_args, deferred, parent_logcontext):
+def s3_download_task(s3_client, bucket, key, extra_args, deferred):


I think these changes make sense. s3_download_task will use whatever the caller logcontext is.

And I think we maintain the logcontext when calling s3_download_task ✅ (at-least with the suggested patterns).

We also make `store_file` and `fetch` async, as they are async in the base class. This also simplifies the implementation. We could go through and convert the whole module from deferreds to async, but that should be done separately.

anoadragon453 · 2025-10-02T10:07:08Z

190686c makes use of ModuleApi.defer_to_thread, which allows modules to easily run a function on a separate thread. However, you must use the default threadpool.

This module previously used its own threadpool (self._s3_pool), and even had an option to configure the size (threadpool_size, default 40). In previous issues, we've suggested users configure boto3's connection count to match the configured threadpool size: #117

The default threadpool size in Twisted is 10. I worry that switching to Synapse's default threadpool will hurt the performance of this module, with no way for users to configure it.

Perhaps we should additionally expose defer_to_threadpool to modules, allowing modules to specify a threadpool? Though this would break compatibility with older versions of Synapse. We could check if the method existed on ModuleApi and use the default threadpool if not.

MadLittleMods · 2025-10-02T15:17:24Z

s3_storage_provider.py

-            s3_download_task(
-                self._get_s3_client(), self.bucket, self.prefix + path, self.extra_args, d, logcontext
-            )
+        await self._module_api.defer_to_thread(


This module previously used its own threadpool (self._s3_pool), and even had an option to configure the size (threadpool_size, default 40). In previous issues, we've suggested users configure boto3's connection count to match the configured threadpool size: #117

The default threadpool size in Twisted is 10. I worry that switching to Synapse's default threadpool will hurt the performance of this module, with no way for users to configure it.

-- @anoadragon453, #134 (comment)

Can we recommend people increase the size of the the default Twisted threadpool?

Is there any difference in having a separate threadpool?

Discussed in #synapse-dev:matrix.org,

Here is why a separate threadpool is important:

I'd be worried about s3 monopolising the thread pool and blocking other operations (like DNS lookups)

-- @richvdh

As @anoadragon453 points out, DNS lookups have their own threadpool already but the point still stands generally.

Can we recommend people increase the size of the the default Twisted threadpool?

We don't currently have such an option in Synapse, but it would be better than nothing.

Perhaps we should try deploying the module with the default threadpool on matrix.org and see if performance suffers? I'm just worried that if we go ahead with not adding any way to configure the threadpool size, yet require people to upgrade this module to use the latest Synapse, they could be stuck between a rock and a hard place.

Otherwise, I think:

Perhaps we should additionally expose defer_to_threadpool to modules, allowing modules to specify a threadpool? Though this would break compatibility with older versions of Synapse. We could check if the method existed on ModuleApi and use the default threadpool if not.

May be the way to go.

Can we recommend people increase the size of the the default Twisted threadpool?

The size of the default Twisted threadpool can only be increased through code, i.e.:

we don't provide a configuration option which tweaks this currently, so users are unable to increase the size of the default threadpool.

Above I suggested exposing X to modules. Another alternative is to add an argument to the already-exposed defer_to_thread ModuleApi method to allow specifying a threadpool, defaulting to the default Twisted threadpool if not provided. It would then use defer_to_threadpool under the hood instead of defer_to_thread.

Sounds workable 👍

I've tried the latter approach in element-hq/synapse#19032.

erikjohnston · 2025-10-03T10:05:11Z

FYI I tried this branch on jki.re with latest develop and no new media is being downloaded. It seems to fail with:

2025-10-03 10:02:43,349 - twisted - 278 - CRITICAL - sentinel - 
Traceback (most recent call last):
  File "/home/erikj/.virtualenvs/synapse311/lib/python3.11/site-packages/s3_storage_provider.py", line 254, in _stream_to_producer
    raise Exception("Timed out waiting to resume")
Exception: Timed out waiting to resume

As introduced by element-hq/synapse#19032.

@erikj

Discovered by @erikj: > the thread is waiting for synapse to use the S3Responder, but the responder isn't returned to Synapse until the thread is finished

> the `d` we pass in to `s3_download_task` gets resolved once we connect to S3, and the thread is concluded only once we finish the download.

anoadragon453 · 2025-10-09T14:16:56Z

Thanks for the reviews both!

My theory as to why this PR passed the integration test CI, but failed on @erikjohnston's homeserver, is that the integration tests do not delete the media from Synapse's local storage after it's uploaded. Thus we don't end up actually fetching the media from the minIO S3 storage upon download.

I'll have a look at modifying the tests to actually do that.

MadLittleMods · 2025-10-09T17:02:24Z

s3_storage_provider.py

+        # DO await on `d`, as it will resolve once a connection to S3 has been
+        # opened. We only want to return to Synapse once we can start streaming
+        # chunks.


Cross-linking internal discussion where the cause of the Exception: Timed out waiting to resume was figured out.

spantaleev · 2025-10-13T09:24:35Z

Using defer_to_threadpool makes this (and s3-storage-provider=v1.6.0, which includes this patch) not compatible with Synapse <1.140.0, because defer_to_threadpool was only introduced (by this patch) in Synapse v1.140.0.

While Synapse v1.140.0rc1 warns about s3-storage-provider=v1.6.0 being required to run that Synapse version, we probably also need a similar warning for s3-storage-provider: you can't run s3-storage-provider=v1.6.0 with older Synapse versions.

Users on the current stable Synapse release (v1.139.0) are better off staying with s3-storage-provider=v1.5.0.

…age_provider_installation_old_boto_workaround_enabled`" This reverts commit 2b0ea94. We're going back to s3-storage-provider=v1.5.0 Ref: matrix-org/synapse-s3-storage-provider#134 (comment)

Ref: matrix-org/synapse-s3-storage-provider#134 (comment) Related to #4635

anoadragon453 · 2025-10-13T09:50:49Z

@spantaleev thanks for raising! We completely forgot to signal that. I've added a warning to the top of the latest release notes: https://github.com/matrix-org/synapse-s3-storage-provider/releases/tag/v1.6.0.

Synapse 1.140.0 is expected to be released tomorrow (as long as no other regressions are found).

anoadragon453 requested a review from a team as a code owner October 1, 2025 16:02

anoadragon453 commented Oct 1, 2025

View reviewed changes

Replace manual LoggingContext usage with ModuleApi.run_in_background

5bdb5d9

Attempt to replace manual usage of LoggingContext with the provided module API's `run_in_background` method.

anoadragon453 force-pushed the anoa/stop_using_logging_context_directly branch from 77b1bc5 to 5bdb5d9 Compare October 1, 2025 16:05

MadLittleMods requested changes Oct 1, 2025

View reviewed changes

Source ModuleApi from hs and use defer_to_thread

190686c

We also make `store_file` and `fetch` async, as they are async in the base class. This also simplifies the implementation. We could go through and convert the whole module from deferreds to async, but that should be done separately.

This was referenced Oct 2, 2025

Provide a default value for server_name to LoggingContext element-hq/synapse#19003

Closed

Fix integration test CI: use auth'd media #135

Merged

Merge branch 'main' into anoa/stop_using_logging_context_directly

5225ce6

MadLittleMods reviewed Oct 2, 2025

View reviewed changes

anoadragon453 mentioned this pull request Oct 7, 2025

Allow Synapse modules to specify their own threadpool when calling defer_to_thread element-hq/synapse#19024

Closed

3 tasks

anoadragon453 changed the title ~~Replace manual LoggingContext usage with ModuleApi.run_in_background~~ Replace manual LoggingContext usage with ModuleApi.defer_to_threadpool Oct 8, 2025

anoadragon453 mentioned this pull request Oct 8, 2025

Expose defer_to_threadpool in the module API element-hq/synapse#19032

Merged

3 tasks

anoadragon453 added 5 commits October 8, 2025 14:17

Use defer_to_threadpool

18b385b

As introduced by element-hq/synapse#19032.

Don't await deferred before passing it back to Synapse

e2abe39

Discovered by @erikj: > the thread is waiting for synapse to use the S3Responder, but the responder isn't returned to Synapse until the thread is finished

await on the correct thing

e9aacbf

> the `d` we pass in to `s3_download_task` gets resolved once we connect to S3, and the thread is concluded only once we finish the download.

Use run_in_background on defer_to_threadpool

7643165

Update documentation to mention dangling coroutines

c882306

anoadragon453 mentioned this pull request Oct 9, 2025

Remove uploaded file from Synapse's local storage during integration tests #137

Draft

erikjohnston approved these changes Oct 9, 2025

View reviewed changes

anoadragon453 merged commit fff398f into main Oct 9, 2025
7 checks passed

anoadragon453 deleted the anoa/stop_using_logging_context_directly branch October 9, 2025 14:15

MadLittleMods reviewed Oct 9, 2025

View reviewed changes

This was referenced Oct 10, 2025

TypeError: LoggingContext.__init__() missing 2 required keyword-only arguments: name and server_name when trying to download media #133

Closed

Import run_in_background correctly from ModuleApi #138

Merged

MadLittleMods mentioned this pull request Oct 10, 2025

Pass run_in_background a callable; don't call it #139

Merged

spantaleev added a commit to spantaleev/matrix-docker-ansible-deploy that referenced this pull request Oct 13, 2025

Revert s3-storage-provider (1.6.0 -> 1.5.0)

f048a0f

Ref: matrix-org/synapse-s3-storage-provider#134 (comment) Related to #4635



		def s3_download_task(s3_client, bucket, key, extra_args, deferred, parent_logcontext):
		def s3_download_task(s3_client, bucket, key, extra_args, deferred):

Replace manual LoggingContext usage with ModuleApi.defer_to_threadpool #134

Replace manual LoggingContext usage with ModuleApi.defer_to_threadpool #134

Conversation

anoadragon453 commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoadragon453 Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston commented Oct 3, 2025

Uh oh!

Uh oh!

anoadragon453 commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spantaleev commented Oct 13, 2025

Uh oh!

anoadragon453 commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Replace manual LoggingContext usage with `ModuleApi.defer_to_threadpool` #134

Replace manual LoggingContext usage with `ModuleApi.defer_to_threadpool` #134

anoadragon453 commented Oct 1, 2025 •

edited

Loading

anoadragon453 commented Oct 2, 2025 •

edited

Loading

MadLittleMods Oct 2, 2025 •

edited

Loading

anoadragon453 Oct 7, 2025 •

edited

Loading