Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor run_in_threadpool to seperate binding from execution #1260

Closed
wants to merge 7 commits into from

Conversation

adriangb
Copy link
Member

@adriangb adriangb commented Aug 7, 2021

This separates the conversion of func to an await able run in threadpool from the actual execution of that awaitable.

This change led to a nice homogenization of the take any sync/async callable/cm and make it an awaitable that FastAPI does: https://github.com/adriangb/anydep/blob/main/anydep/concurrency.py

So while this has no immediate advantage for Starlette, I think it gives developers of downstream packages more flexibility, and it also has no negative impact on Starlette.

@JayH5
Copy link
Member

JayH5 commented Aug 16, 2021

Hey 👋 these PRs around the concurrency stuff seem potentially like useful changes, but I think we'd be better off discussing them in an issue or discussion.

This PR in particular we're unlikely to merge since we'd actually like to reduce the API surface of the concurrency module (possibly even making it an internal module at some point).

@adriangb
Copy link
Member Author

adriangb commented Aug 16, 2021

Makes sense to discuss. Do you want me to open an issue given that we already have this PR?

Reducing the surface area makes sense, I agree that these sort of utilities are best kept as internal. Might be a bit late though given that downstream packages like FastAPI already depend heavily on this module.

For this PR in particular, how about making the new method a private _ method? It doesn't help downstream packages but it might help Starlette internally and eventually lead to deprecation and replacement of the current run_in_threadpool method.

That said, this PR increases the surface area by 1 method, but also reduces an existing one to a 1 line wrapper, so it's practically a net zero change

@adriangb
Copy link
Member Author

I'll close this as it doesn't seem likely to get merged and doesn't really add any value to Starlette itself

@adriangb adriangb closed this Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants