async_unsafe's get_event_loop usage leads to "Too many open files" in tests

[patrick91]

Hi folks, I was going to write this in django developers, but looks like I might have found a small area for improvement for the async_unsafe decorator, but first let me give you a bit of background:

We're upgrading to Django 3.0 and we've been encountering this error on ci when running all the tests: "OSError: [Errno 24] Too many open files".

Here's the full stack trace:  https://dpaste.com/ABYJFP6CR

By inspecting it and the source code it seems to be related to the async_unsafe decorator: https://github.com/django/django/blob/855fc06236630464055b4f9ea422c68a07c6d02a/django/utils/asyncio.py#L19

This decorator calls `get_event_loop`: https://github.com/python/cpython/blob/3.7/Lib/asyncio/events.py#L632

which creates a new loop every time, which seems to be the cause of the too many files open. Wouldn't be best to use `get_running_loop` https://github.com/python/cpython/blob/3.7/Lib/asyncio/events.py#L681 ?

We are already checking for RuntimeError so it should be an easy fix, but I wonder if the usage of `get_event_loop` is intentional and I'm missing something here.

Happy to provide a patch for this :)

Patrick

[Adam Johnson]

 I was going to write this in django developers

This is django-developers?

which creates a new loop every time, which seems to be the cause of the too many files open.

Can you give a bit more explanation how it creates a new loop every time?

Two successive calls to get_event_loop() in ipython give me the same object:

In [4]: l = asyncio.get_event_loop()

In [5]: l2 = asyncio.get_event_loop()

In [6]: id(l)
Out[6]: 4561753280

In [7]: id(l2)
Out[7]: 4561753280

Additionally, the docs for get_running_event_loop() say:

This function can only be called from a coroutine or a callback.

Plus it was added in Python 3.7, whilst Django 3.1 supports 3.6+. So it's not clear we can use it.

That said, it would be nice to avoid the overhead of creating an event loop for sync-only use cases.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/a7cedcc5-b041-4f21-9a22-e9ffb92d3c91n%40googlegroups.com.

--

Adam

[Patrick Arminio]

On Thu, 8 Oct 2020 at 22:28, Adam Johnson <m...@adamj.eu> wrote:

 I was going to write this in django developers

This is django-developers?

I meant django-users, sorry :D

 

which creates a new loop every time, which seems to be the cause of the too many files open.

Can you give a bit more explanation how it creates a new loop every time?

Uhm, I'll try to investigate a bit more tomorrow, we have a CI failure where ~152 fail due to the same issue,

I wonder if I can set up a small test suite that can reproduce this. But I guess every test runs the same

Two successive calls to get_event_loop() in ipython give me the same object:

In [4]: l = asyncio.get_event_loop()

In [5]: l2 = asyncio.get_event_loop()

In [6]: id(l)
Out[6]: 4561753280

In [7]: id(l2)
Out[7]: 4561753280

Additionally, the docs for get_running_event_loop() say:

This function can only be called from a coroutine or a callback.

That's why we need the RuntimeError check, no?

 

Plus it was added in Python 3.7, whilst Django 3.1 supports 3.6+. So it's not clear we can use it.

Oh, this might be a problem considering the implementation of `get_running_event_loop` is written in C,

not sure it can be backported easily.

 

That said, it would be nice to avoid the overhead of creating an event loop for sync-only use cases.

Yup :)

 

On Thu, 8 Oct 2020 at 21:28, patrick91 <patrick...@gmail.com> wrote:

Hi folks, I was going to write this in django developers, but looks like I might have found a small area for improvement for the async_unsafe decorator, but first let me give you a bit of background:

We're upgrading to Django 3.0 and we've been encountering this error on ci when running all the tests: "OSError: [Errno 24] Too many open files".

Here's the full stack trace:  https://dpaste.com/ABYJFP6CR

By inspecting it and the source code it seems to be related to the async_unsafe decorator: https://github.com/django/django/blob/855fc06236630464055b4f9ea422c68a07c6d02a/django/utils/asyncio.py#L19

This decorator calls `get_event_loop`: https://github.com/python/cpython/blob/3.7/Lib/asyncio/events.py#L632

which creates a new loop every time, which seems to be the cause of the too many files open. Wouldn't be best to use `get_running_loop` https://github.com/python/cpython/blob/3.7/Lib/asyncio/events.py#L681 ?

We are already checking for RuntimeError so it should be an easy fix, but I wonder if the usage of `get_event_loop` is intentional and I'm missing something here.

Happy to provide a patch for this :)

Patrick

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/a7cedcc5-b041-4f21-9a22-e9ffb92d3c91n%40googlegroups.com.

--

Adam

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM2_jHz54gfwcqwib9G%2BfxKxh9HER8dYJjJ4G1hNxvysuw%40mail.gmail.com.

--

Patrick Arminio

[Adam Johnson]

That's why we need the RuntimeError check, no?

I don't see how RuntimeError can be raised. The docs I spoke about say get_running_loop() cannot be used synchronously, not get_event_loop() .

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAOUxZct1YqydzG%3DB8hkzq1WFUhYFrjq7Ag3xw-3hDmSr79HBjw%40mail.gmail.com.

--

Adam

[Patrick Arminio]

On Fri, 9 Oct 2020 at 10:40, Adam Johnson <m...@adamj.eu> wrote:

That's why we need the RuntimeError check, no?

I don't see how RuntimeError can be raised. The docs I spoke about say get_running_loop() cannot be used synchronously, not get_event_loop() .

We still check for RuntimeErrors though: https://github.com/django/django/blob/855fc06236630464055b4f9ea422c68a07c6d02a/django/utils/asyncio.py#L19

So I think using (if we could) `get_running_loop` would be safe, since we want to check if we are running under asyncio.

Maybe I can get back to this "issue" in future, when support for python 3.6 is dropped. Doesn't look like it is something that's bothering many people anyway :)

For now we had this issue only during tests, so we disabled the check. We might get back to it when we start using async.

I'll report any useful findings :)

 

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM0HXqTCxc0cZF_sVQ%3Dvu3hNOKL9G2E%3Dr5pFPTEbLPjF3g%40mail.gmail.com.

--

Patrick Arminio

[Adam Johnson]

I'd like to see what Andrew thinks on this.

I had a thought that the issue you're seeing may be related to a dependency of your project creating/destroying event loops in the background. If you could try and reproduce it that'd be neat.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAOUxZcuanf75h8u7mzU%3D1GXZZTXoz84N9kpbWemeZF3hfr-NKw%40mail.gmail.com.

--

Adam

[Andrew Godwin]

Yeah, I'm not quite sure what this is, but I just ran get_event_loop() a thousand times and it gave me the same thing every time and didn't even budge the number of file descriptors.

Can you replicate this behaviour in a brand new Django project? That's what I'd need to help debug it further.

Andrew

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM37BpLrTs90H0YyWDtoH%3Dak0%2Bb1peX%3DX0dfE14Mz5PeGA%40mail.gmail.com.

[Patrick Arminio]

On Tue, 13 Oct 2020 at 19:07, Andrew Godwin <and...@aeracode.org> wrote:

Yeah, I'm not quite sure what this is, but I just ran get_event_loop() a thousand times and it gave me the same thing every time and didn't even budge the number of file descriptors.

Can you replicate this behaviour in a brand new Django project? That's what I'd need to help debug it further.

I'll try but can't promise anything :D our project (and tests) setup is quite complex  :)

 

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/12b4011d-7a7d-4fb5-8225-f758b3478e37%40www.fastmail.com.

--

Patrick Arminio