Fix sql recording for async views

[bellini666]

No description provided.

[tim-schilling]

It's good to see this. We'd likely need to add a test using an async view to verify this and prevent regressions.

[bellini666]

@tim-schilling yeah, I can do that.

Will check even why some tests failed saying that ContextVar is not callable, since it totally is (maybe something related to the mocks?).

For context, I did that change in strawberry-django-plus to make sql recording work.

The only issue other than this one is that I had to disable the Template recorder. Specifically when loading the graphiql template, it triggers a database query to get current user which is not async safe (the issue does not happen if not logged in).

[tim-schilling]

Hmm, I wonder if we could replace more usages of threading with contextvars.

[matthiask]

Hmm, I wonder if we could replace more usages of threading with contextvars.

Yes, or even with asgiref.local.Local.

[bellini666]

Hrm, interesting... Didn't know about asgiref.local.Local. Are there any advantages to it? AFAIK contextvars is basically a superior implementation that works for both threading and asyncio.

The only moment it could not be used is on python 3.6 or less, since it was released in python 3.7, but it has already reached EOL so it should not be a problem.

[bellini666]

Hey @tim-schilling , just added some tests. Don't know why postgres/postgis tests are failing though

[tim-schilling]

@matthiask I decided to copy the database_sync_to_async function from the channels project rather than add that as a dependency to our testing utilities. When the toolbar formally supports channels, we can switch that around.

[matthiask]

@matthiask I decided to copy the database_sync_to_async function from the channels project rather than add that as a dependency to our testing utilities. When the toolbar formally supports channels, we can switch that around.

Great! Maybe the relevant utility will be added to Django itself soon. I'm not following the async database work too closely but something is happening there.

[tim-schilling]

I think this is good unless we also want to switch over the other panels to use ContextVar.

[bellini666]

Hey guys! Just checking if there's anything missing in this PR?

[tim-schilling]

@matthiask I'm good with this. What do you think?

[matthiask]

Looks good to me. I haven't tested it myself and do not have much experience with async code so I still have some uncertainty, but let's move ahead with this. Thanks @bellini666 and @tim-schilling !