ASGI_THREADS environment variable has no effect since 2.4.1

[revoteon]

Channels documentation mentions that:

By default, the number of threads is set to “the number of CPUs * 5”, so you may see up to this number of threads. If you want to change it, set the ASGI_THREADS environment variable to the maximum number you wish to allow.

This behaviour used to work correctly before Daphne v2.4.1.

Since 2.4.1 setting ASGI_THREADS envvar has no effect.

My Setup

Python 3.7.5, on Ubuntu 19.10 with following packages:

asgiref==3.2.7
channels==2.4.0
channels-redis==2.4.2
daphne==2.4.1
Django==2.2.12

Startup:

daphne -b 0.0.0.0 -p 8000 api.config.asgi:application

How to replicate

• Set ASGI_THREADS to 1.
• Install packages above, with Daphne v2.40
• Start daphne
• Navigate through website and open a few pages
• Check number of threads with htop.
• Thread count never exceeds 1
• Now install daphne v2.41
• Start server and browse website
• Thread count is not limited to 1, keeps increasing. Possibly capped to 5*CPU count, but can't tell for sure.

[JohnDoee]

Nothing related to that was changed in 2.4.1, that stuff is all managed by asgiref.

The thread stuff is a lot better in python 3.8 but imo the whole ThreadPoolExecutor design is terrible.

[revoteon]

@JohnDoee It did, it is this commit 27f760a. Reverting this change resolves the issue.

[JohnDoee]

I tested it, sorry, you're right. I assumed the correct loop was installed as global correctly. It seems like the "correct" ThreadPoolExecutor is installed on the wrong loop.

[carltongibson]

Hmmm. Yes. It's because SyncToAsync is using get_event_loop() which is returning the default event loop for the main thread, rather than the loop we're passing to twisted (for use in the running thread).

[JohnDoee]

I assumed that https://github.com/django/daphne/blob/2.4.1/daphne/server.py#L129 would make it all correct but maybe stuff is executed in the wrong order.

[JohnDoee]

I've been testing this a bit further, this actually breaks everything that uses asyncio and is initialized at the "wrong" time. The original loop seems to be stopped.

Now I have this piece of boilerplate code at the top somewhere imported early to make sure everything else uses the correct loop.

import asyncio  # isort:skip
import daphne.server  # isort:skip
from twisted.internet import reactor  # isort:skip
asyncio.set_event_loop(reactor._asyncioEventloop)  # isort:skip

[carltongibson]

See django/asgiref#185 (comment):

We need to resolve, probably reverting 27f760a — but then that recreates django/channels#1374 — so it's currently pending time to think about it.

[carltongibson]

#299 is related.

[apollo13]

So I have been looking through asgiref, channels daphne etc… and I came to the following conclusion:

Remove ASGI_THREADS support from asgiref. Before you say no, hear me out…

• asgiref is a low level library which basically only requires an event loop to be running, it should not concern itself with more.
• It does not know from which thread it is imported and as such it shouldn't make assumption about the event loop there,
• get_event_loop which is currently used is slated for deprecation and it appears as if only get_running_loop will be supported going forward -- which is not something that will exist during import time usually.
• Even if the event loop where running and we were able to get it at import time, does it sound safe to change the associated thread pool while the event loop is running? (does python even support that?)

Okay, now with ASGI_THREADS removed we need to bring back the functionality. How do we do that? As stopgap measure we can simply add it to daphne/server.py right after we call new_event_loop -- it is our event loop after all, so let's see the pool there. Going further we can deprecate ASGI_THREADS completely and implement this in the ASGI servers them self, they are the ones in control (and owners) of the loop after all. So for daphne I'd expect support for --worker-threads (or so…), which manage.py runserver from channels could also use then.

What do you think?

[apollo13]

See django/asgiref#185 (comment):

We need to resolve, probably reverting 27f760a — but then that recreates django/channels#1374 — so it's currently pending time to think about it.

@carltongibson:
I started to think about that and ran over your comment in django/channels#1374 (comment) -- when I tried this today on Python 3.10 and up2date asgiref/django I did not get INCORRECTLY raised SynchronousOnlyOperation but only:

/home/florian/sources/async/test3.py:39: DeprecationWarning: There is no current event loop
  target=run_in_async_thread, args=(asyncio.get_event_loop(),), kwargs={}, name='async-thread'
Async thread starting
Will start the loop
About to call sync_only from async context
Correctly raised SynchronousOnlyOperation
About to call sync_only from SYNC context
sync_only successfully called.

with that in mind it might (?) be possible to actually revert the changes. That said I would recommend against reverting because creating our own event loop and configuring it as needed sounds like the way to go… Nevertheless I'd be interested to know if you have an idea whether python fixed something or we fixed a few bugs in asgiref along the way…

[carltongibson]

Hey @apollo13 — thanks for digging. Current status is I don't know.

My plan (since late Summer) has been to sit down with Daphne and resolve this issue properly. Life has intervened but, it's my top item (having just got 4.0 compatibility going for django-compressor). So reality is more work (from me) on the Channels Trio in the new year now.

However, if you're digging actively I am very happy to work with you on that.

That said I would recommend against reverting because creating our own event loop and configuring it as needed sounds like the way to go…

Yes, it seemed like a good idea. The issue was that it turns out that you need to set the event loop in your thread if you're not using the default, and whilst that's possible it creates a bookkeeping overhead, that folks don't want to do.

I began a conversation with @orf on the Forum about not using thread for the auto-reloader that was prompted by this issue. https://forum.djangoproject.com/t/feasibility-of-using-subprocess-for-auto-reloader/4970 — That's the way I'd like to go (but that's a lot of Yak...)

[apollo13]

Hi @carltongibson, while it is true that changes to the autoreloader might be possible; I still think that ASGI_THREADS doesn't belong into asgiref and we should fix that independent of further improvements in the autoreloader, ie what I outlined in #319 (comment) -- is that something we would be okay with or a big nogo (I am asking because this is basically the first time doing anything with asgi and as such I am not very up2date on it's history)

[carltongibson]

what I outlined in #319 (comment)

Certainly happy for Daphne to have that. There needs to be some hard upper limit somewhere, and the protocol server seems the right level for that.

(I can't recall without looking if there are other uses of ASGI_THREADS in asgiref that would allow dropping it entirely there.)

[carltongibson]

Nevertheless I'd be interested to know if you have an idea whether python fixed something or we fixed a few bugs in asgiref along the way…

OK, so I finally had the combination of time/energy/health to look into this (and the related cluster of issues) again. The bottom-line is that I think dropping PY36, and get_event_loop() in favour of get_running_loop() across the board (and particularly in asgiref.sync) means that this issue is likely Gone Away™ (for some value of that).

• Using get_running_loop() will get the right event loop, generally
• We already call set_event_loop in the Server, so making sure we did similar in Worker scripts would likely be enough.
• Given that, it may be that we don't need to worry about the reloader running in a sub-thread (not sure yet)
• But I tried deferring the twisted reactor setup in server.py, which doesn't look possible.
• So, maybe, I can rework the Channels runserver to run with a subprocess if that's still needed.

That's very preliminary. (I only had the session today to look at it) But maybe the pandemic induced don't look at it for two years has paid off. I'll keep working on this, but it feels like what was intractable now at least has a way of being untangled.

[carltongibson]

OK, so step one is RFC to remove ASGI_THREADS from asgiref: django/asgiref#336

Then we'll bring similar in here to restore this functionality to Daphne.

[carltongibson]

OK, initial port of the environment variable handling in #422.

[aeyoll]

Hello @carltongibson,
Is the fix in #422 going to be backported to the 2.x branch? The 2.x documentation for ASGI_THREADS still mentions this feature.

Thanks a lot

[carltongibson]

@aeyoll no. You'd be welcome to backport it yourself and deploy that patched versions, or else update to the new version. (Beta on PyPI now. Final in a couple of weeks.)

[aeyoll]

Thanks for the answer! Maybe it should be pertinent to add a note on the documentation, specifying the versions where it doesn't work.