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

Allow passing a custom loop setup function instead of None #2391

Conversation

gnir-work
Copy link

@gnir-work gnir-work commented Jul 16, 2024

Summary

Support passing a custom IOLoop setup function in order to support running uvicorn in custom ioloop implementation other that the default and uvloop.

For example this feature will allow using this monitored IOLoop inside uvicorn applications :)

Link to discussions thread - #2256

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@gnir-work gnir-work force-pushed the feature/support-passting-custom-ioloop-setup-function branch 5 times, most recently from 95668cf to 4ef3fd3 Compare August 17, 2024 19:17
@gnir-work gnir-work force-pushed the feature/support-passting-custom-ioloop-setup-function branch from 4ef3fd3 to 0b14e3c Compare August 17, 2024 19:24
@gnir-work gnir-work marked this pull request as ready for review August 17, 2024 19:28
@Kludex
Copy link
Member

Kludex commented Aug 18, 2024

@gnir-work I don't want to have 2 similar parameters. I'd prefer us to go with this approach: https://github.com/encode/uvicorn/pull/2130/files (which turns the --loop into your --loop-setup.

If you want to move this forward, the best approach would be to continue that PR - fixing the pipeline, etc.

@hxnir
Copy link

hxnir commented Aug 18, 2024

No problem :)
Just a quick question - How should I go about it? Should I fork the graingert:uvicorn repo and open another PR?
I can also update the current diff to be in the same spirit as the PR you mentioned

@Kludex
Copy link
Member

Kludex commented Aug 18, 2024

I would add a remote on his repo, get the branch, and push to your fork, and then create a PR.

@gnir-work gnir-work mentioned this pull request Aug 18, 2024
3 tasks
@gnir-work
Copy link
Author

Closing this PR in favor of #2435.
Will update it from draft to open as soon as everything is passing :)

@gnir-work gnir-work closed this Aug 18, 2024
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.

3 participants