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

Prevent the debug server from setting up its own event loop #945

Conversation

DoctorJohn
Copy link
Member

Description

This PR is a possible fix for #940. It configures uvicorn to not create its own event loop. This way the strawberry.dataloader.DataLoader.loop would be the same as the loop used by uvicorn.

While I would probably not configure uvicorn the same way in production code, I think it's the simplest (maybe the only) way to make dataloaders work with the debug server command.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@DoctorJohn DoctorJohn force-pushed the let-the-debug-server-reuse-the-default-event-loop branch from 338005b to 0247153 Compare May 17, 2021 22:05
@DoctorJohn DoctorJohn changed the title Prevent uvicorn from setting up its own event loop Prevent the debug server from setting up its own event loop May 17, 2021
@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #945 (29bc552) into main (4fc9c9b) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #945      +/-   ##
==========================================
+ Coverage   97.41%   97.52%   +0.10%     
==========================================
  Files          76       76              
  Lines        2592     2705     +113     
  Branches      362      378      +16     
==========================================
+ Hits         2525     2638     +113     
  Misses         41       41              
  Partials       26       26              

@@ -57,4 +57,4 @@ def server(schema, host, port, app_dir):
app.add_websocket_route(path, graphql_app)

print(f"Running strawberry on http://{host}:{port}/ 🍓")
uvicorn.run(app, host=host, port=port, log_level="error")
uvicorn.run(app, loop="none", host=host, port=port, log_level="error")
Copy link
Member

Choose a reason for hiding this comment

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

Is this documented in the uvicorn docs? I was only able to find it here: https://github.com/encode/uvicorn/blob/bf1c64e2c141971c546671c7dc91b8ccf0afeb7d/uvicorn/config.py#L55-L60

I'll test this with the dataloader example and see if it works :)

Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

It appears to be undocumented on their website which matches the uvicorn cli help command. Maybe it was intended for programmatically usage only. However, according to this PR they added it specifically to allow the setup of custom event loops.

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Just tested and it seems to work! Good spot!

@botberry
Copy link
Member

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This releases fixes an issue with the debug server that prevented the
usage of dataloaders, see: #940

@patrick91 patrick91 merged commit 8130a49 into strawberry-graphql:main May 19, 2021
@DoctorJohn DoctorJohn deleted the let-the-debug-server-reuse-the-default-event-loop branch November 20, 2024 16:03
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.

Can't get DataLoader to work
3 participants