Skip to content
New issue

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

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

Already on GitHub? # to your account

check_request_enabled.send() should not be called from an async context #915

Open
1 task done
Ariki opened this issue Dec 11, 2023 · 3 comments
Open
1 task done

Comments

@Ariki
Copy link

Ariki commented Dec 11, 2023

Understanding CORS

  • I have read the resources.

Python Version

3.11

Django Version

4.2

Package Version

4.3.1

Description

The current CorsMiddleware implementation can work both in sync and async mode. The issue is that even in async mode, it uses Django signal's send method which isn't intended to be used from an async context.

This kind of works in simple cases but you cannot use Django ORM functions from a signal handler called in an asynchronous context.

Starting from version 5.0, Django supports asynchronous signal handlers and introduces the asend method which can be used to send signals from asynchronous code.

So I believe the correct approach would be as follows:

  • use send if the middleware is run in synchronous mode;
  • wrap send to sync_to_async() if the middleware is run in asynchronous mode, and Django version is < 5.0;
  • use asend if the middleware is run in asynchronous mode, and Django version is >=5.0.

Some refactoring is needed for this to work, as await can be used only in async methods.

@adamchainz
Copy link
Owner

There’s no edict that the synchronous send() function should not be called from an asynchronous context. It’s legitimate but it does prevent the receivers from being asynchronous.

I don’t think we can remove the call as that is backwards incompatible. Maybe we can add an asend() call, though that would require significant restructuring.

the signal is my least favourite feature. It was added before I took over maintenance and I haven’t removed it due to backwards compatibility concerns. In most cases I think subclassjng the middleware makes more sense, though it’s not documented or well structured. If you have a specific use case, perhaps we can find a way to support that through subclassing.

@Ariki
Copy link
Author

Ariki commented Dec 19, 2023

It’s legitimate but it does prevent the receivers from being asynchronous.

It also prevents the receivers from accessing the database (which is my use case; my code was broken after upgrading from an older version of django-cors-headers).

But I'm ok with subclassing. My only concern is that I had to copy&paste the whole add_response_headers method into my subclass just to make its async version and be able to use await in the middle of it.

@adamchainz
Copy link
Owner

I'd be open to proposed restructuring to make subclassing easier, but I don't have time to look at it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants