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

Use Django async-native APIs where possible #2089

Open
bigfootjon opened this issue Apr 14, 2024 · 9 comments
Open

Use Django async-native APIs where possible #2089

bigfootjon opened this issue Apr 14, 2024 · 9 comments

Comments

@bigfootjon
Copy link
Collaborator

As discussed on the Django forumns Carlton pointed out that one area we'd like to invest in in is Channels support for the new async functionality that has been added over the last several releases (refer to the above thread for details).

I've scanned the channels repo by searching for database_sync_to_async and observed a few areas that could benefit from these new APIs:

  1. Sessions
  2. Auth

(this list is the same as Carlton pointed out on that thread, something tells me he might be some kind of expert!)

The other calls to database_sync_to_async are from wrappers for intentionally sync code, or tests (neither of which are pertinent).

I'd like to propose using these native APIs. The one hitch I see is that these features are too new to be used presently:

  1. async sessions has not yet been released (scheduled for 5.1)
  2. async auth was released in 5.0

Considering that Channels currently supports 4.2 (and definitely doesn't yet support unreleased 5.1) we might have to do some feature-checking or wait until Channels bumps its minimum supported version. Should I start now with writing some polyfills/feature-checks or set this aside until minimum versions are bumped?

@carltongibson
Copy link
Member

Hey @bigfootjon — thanks for this.

So, yes, we should definitely leverage the new APIs where we can.

The standard approach would be to try and import the new code (or branch on Django version number), use it if it's available, otherwise fallback to our existing code.

We won't drop 4.2 support until 5.2 is released (and maybe a teeny bit after that) but that's no reason we can't get folks on the latest versions updated.

We test against Django main branch, which is already 5.1 (although pre-alpha), so no-reason not to already begin with the code for the upcoming version.

@cclauss
Copy link
Contributor

cclauss commented Apr 14, 2024

Perhaps separate pull requests for sessions and auth to simplify testing and code review and also to provide more flexibility in deploying the PRs.

@bigfootjon
Copy link
Collaborator Author

Sounds good, I'll put up 2 different PRs and import testing to see if the new features exist.

While figuring out what would be needed to do that I noticed some bits in the docs that need to be updated for async django ORM support: #2090

@bigfootjon
Copy link
Collaborator Author

Here we go:

  1. Sessions PR: Use the async session.asave method if it exists #2092
  2. Auth PR: Use the async auth api methods if they exist #2093

I also noticed an out-of-date fallback import in #2091

@sevdog
Copy link
Contributor

sevdog commented Aug 14, 2024

The usage of django async method is very useful, also because it is easier to write tests with django built-in testcases.

My concern is: currently there is nothing in the flow of channels which performs database connection cleanup while django uses request_started/request_finished signals to perform such task:

https://github.com/django/django/blob/43cdfa8b20e567a801b7d0a09ec67ddd062d5ea4/django/db/__init__.py#L62-L63

IMO the only good part left in database_sync_to_async is to invoke the close_old_connections, without it how are we going to clean-up those connections?

@bigfootjon
Copy link
Collaborator Author

@sevdog thats actually changed recently. Take a look at this PR: #2090

@sevdog
Copy link
Contributor

sevdog commented Aug 14, 2024

Thank you @bigfootjon, I missed that PR. Nice job.

@carltongibson
Copy link
Member

And now we just need to push forward these last few bits and get the new version out.

@bigfootjon I'm fairly sure I'm blocking something. Just let me know and I'll get to it 🫡

@bigfootjon
Copy link
Collaborator Author

Actually we’re blocked on me having a bit of time to review other PRs! The auth PR will have to wait for Django 5.2 (since the underlying change it depends on in core make the 5.1 cut)

I’ve got a few other changes I’m trying to fit into this release, I’ll tag you on them when they’re ready for a final review!

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

No branches or pull requests

4 participants