Skip to content

Conversation

@anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Oct 7, 2025

Spurred on by matrix-org/synapse-s3-storage-provider#134 (comment). Should be backwards-compatible with all modules.

Superseded by #19032.

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)

Added in Synapse v1.49.0.
Args:
Copy link
Contributor

Choose a reason for hiding this comment

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

Update docstring and details about the default

async def defer_to_thread(
self,
f: Callable[P, T],
threadpool: Optional[ThreadPool] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will mess with positional args that people are already using with defer_to_thread.

For example: module_api.defer_to_thread(add, 1, 2) will be have 1 interpreted as the threadpool positional argument.

We could use a keyword argument and hope there aren't arg naming collisions with threadpool with the f function. We could further namespace the variable name like defer_to_thread_threadpool

But I think it might be better if we just introduce defer_to_threadpool to the ModuleApi. We need to rely on a new version of the ModuleApi anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Argh, yes, good point.

While writing this PR, it also felt like a code smell to have the ModuleApi's defer_to_thread actually be defer_to_threadpool under the hood.

I'll rework this PR to expose defer_to_threadpool instead.

@anoadragon453
Copy link
Member Author

I've ended up replacing this PR with #19032, which exposes defer_to_threadpool instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants