Use a new loop in run_app()

[Dreamsorcerer]

run_app() currently uses get_event_loop(), and basically relies on the side-effect of it creating a new loop. This causes issues if running code beforehand, for example with asyncio.run() as the side-effect will no longer happen and a RuntimeError is raised.

As the function is going to run the loop, there is no reason to think there would be a currently running loop, so I think it's safest to just create a new loop and set it as the current loop.

[webknjaz]

I'm not sure about this. The idea has always been that the current implicit loop gets used. OTOH the cleanup code resets the loop so maybe it's fine to do the same at the beginning too...

[Dreamsorcerer]

OTOH the cleanup code resets the loop so maybe it's fine to do the same at the beginning too...

Do you mean asyncio.set_event_loop(None)? That has no effect on this issue. Another result of this, is that it is impossible to use run_app() twice in a row (without manually setting a new loop between the calls).

The idea has always been that the current implicit loop gets used.

This seems odd to me. It gets the implicit loop, and then decides that it owns it? So, it then closes and resets the current loop. That seems surprising to me. If it wants to own a loop, then it should create that loop.

Overall, I expect the function to behave much like asyncio.run(). In fact, the try/finally block looks like it was copied from that function. But, asyncio.run() creates a new loop:
https://github.com/python/cpython/blob/master/Lib/asyncio/runners.py#L39

[webknjaz]

I don't think it's a good idea to have it here. At least, put it right before try:. The reason is that it's an antipattern to have more than one instruction in a try block (it tends to shadow exceptions sometimes and causes hard-to-debug situations).

[Dreamsorcerer]

Yeah, I just noticed that it was inside the try block in asyncio.run(), figured there was probably a good reason for it.

[webknjaz]

Well, you could add a separate try/except if you want to call loop.close() and re-raise. There's no need to asyncio.set_event_loop(None) because setting it didn't happen anyway.

[Dreamsorcerer]

I've kept these 2 lines together in the try to match https://github.com/python/cpython/blob/main/Lib/asyncio/runners.py#L40-L44

But, I've moved the create_task() to before the try statement, as it doesn't seem to have any business being there.

[webknjaz]

This seems odd to me. It gets the implicit loop, and then decides that it owns it? So, it then closes and resets the current loop. That seems surprising to me. If it wants to own a loop, then it should create that loop.

The more I think about it, the more it seems like Andrew was talking about non-run_app() cases. His point was mostly that the current loop should be autodetected except that tests needed their own per-test loops.

[codecov]

Codecov Report

Merging #5572 (cdf1835) into master (fd2ea56) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5572   +/-   ##
=======================================
  Coverage   96.75%   96.75%           
=======================================
  Files          44       44           
  Lines        9847     9849    +2     
  Branches     1590     1591    +1     
=======================================
+ Hits         9527     9529    +2     
  Misses        182      182           
  Partials      138      138           

Flag	Coverage Δ
unit	96.64% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/web.py	99.15% <100.00%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd2ea56...cdf1835. Read the comment docs.

[Dreamsorcerer]

Think this should be ready now.

[webknjaz]

Side note: ouch, this looks awfully hardcoded. It's probably a good idea to fix such hardcoded socket paths across tests (in a dedicated PR, of course).

[patchback]

Backport to 3.8: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 88d1f80 on top of patchback/backports/3.8/88d1f801903e132eca9736baf351861d60a9b4ad/pr-5572

Backporting merged PR #5572 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git add remote upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.8/88d1f801903e132eca9736baf351861d60a9b4ad/pr-5572 upstream/3.8

4. Now, cherry-pick PR Use a new loop in run_app() #5572 contents into that branch:

   $ git cherry-pick -x 88d1f801903e132eca9736baf351861d60a9b4ad

   If it'll yell at you with something like fatal: Commit 88d1f801903e132eca9736baf351861d60a9b4ad is a merge but no -m option was given., add -m 1 as follows intead:

   $ git cherry-pick -m1 -x 88d1f801903e132eca9736baf351861d60a9b4ad

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Use a new loop in run_app() #5572 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.8/88d1f801903e132eca9736baf351861d60a9b4ad/pr-5572

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[webknjaz]

@Dreamsorcerer would you backport this?

[krizex]

This change is NOT backward compatible and my App just get broken.

If your app creates some asyncio tasks or store the current event loop for future usage before calling the app.run without passing the loop parameter, it is likely your app will be broken after upgrading aiohttp to 3.8.0.

[asvetlov]

Sorry for that, it is your oversight. The feature reversion in 3.8 makes even more mess, unfortunately.
If you cannot upgrade to 3.8 -- please stay with 3.7

[Dreamsorcerer]

If your app creates some asyncio tasks or store the current event loop for future usage before calling the app.run without passing the loop parameter, it is likely your app will be broken after upgrading aiohttp to 3.8.0.

Yep, sorry, but it does sound like a very precarious way to write an application. I mean, you can't even use asyncio.create_task() without a running event loop. run_app() should behave the same way as asyncio.run() and be the main entry point for your application. I'd suggest whatever tasks etc. you are creating should be done in the application setup and added as startup/cleanup_ctx functions.

[krizex]

@Dreamsorcerer Actually the event loop in our app does not run aiohttp only, it also executes some other coroutines which are created before running the aiohttp, and we also defined a class level aiodns.DNSResolver instance which stores the current event loop to its member. Before 3.8.0 there is no loop parameter for run_app(), and aiohttp uses the current event loop. So I suppose in 3.8.0 aiohttp should also use the current event loop if it exists when the loop is None.

[SamuelGong]

@Dreamsorcerer Actually the event loop in our app does not run aiohttp only, it also executes some other coroutines which are created before running the aiohttp, and we also defined a class level aiodns.DNSResolver instance which stores the current event loop to its member. Before 3.8.0 there is no loop parameter for run_app(), and aiohttp uses the current event loop. So I suppose in 3.8.0 aiohttp should also use the current event loop if it exists when the loop is None.

I second that!

[webknjaz]

I'd say that run_app() is a convenience high-level method for running an aiohttp app. If you want to have more control, just use lower level APIs.

[asvetlov]

Maybe we need async function that does almost all run_app() functionality but does nothing with loop creation and running.

E.g. asyncio.run(async_run_app(app_maker()))

I recall a similar future request in the past.

[Dreamsorcerer]

Maybe we need async function that does almost all run_app() functionality but does nothing with loop creation and running.

Possibly, but I've had similar discussions in other bug reports where users say they run other things in addition to the aiohttp app, just like @krizex. So far, everytime someone has actually elaborated on what they are doing, it turns out their code is brittle and should have been written with the aiohttp app being in full control and everything else running from application startup/cleanup_ctx.

So, maybe this is actually more of a documentation issue, to encourage users to better understand how complex applications should be written.

As an example, one of my apps requires a second server, which when running locally, I spawn in a separate process. The code for that looks something like:

async def run_server(_app):
    proc = await asyncio.create_subprocess_exec(path)

    yield

    if proc.returncode is None:
        proc.terminate()
    await proc.wait()

app.cleanup_ctx.append(run_server)

If you need to run a long coroutine, then maybe something like:

async def run_other_task(_app):
    task = asyncio.create_task(other_long_task())

    yield

    task.cancel()
    with suppress(asyncio.CancelledError):
        await task  # Ensure any exceptions etc. are raised.

Not doing it this way creates issues, with the app object not being in control of its own lifecycle, devtools failing to reload etc. I also can't see how you can write the code without using the low-level asyncio API, which is also discouraged. The above examples only use the high-level API.

[Dreamsorcerer]

I think there's just confusion over how to write these kind of apps. It's the equivalent of writing a general asyncio application something like:

loop = asyncio.get_event_loop()
loop.create_task(do_one_thing())
loop.run_until_complete(app_task())

This is heavily discouraged and should obviously be written without the low-level API, using a main() function like:

async def main():
    asyncio.create_task(do_one_thing())
    await app_task()

asyncio.run(main())

In an aiohttp application, the app object becomes our main() function, which I think is what most users are missing.

[asvetlov]

I'm ok with any decision, and don't want to invest my time in writing async_run_app().
But, if somebody creates a PR, I will review it.

[vmarkovtsev]

My application has broken, too. I would expect this kind of change in 4.0, according to semver best practices. I have nothing against the idea, but I am against backward-incompatible changes within the same major version.

I mean, you can't even use asyncio.create_task() without a running event loop

I am using asyncio.ensure_future that works.

[asvetlov]

I'm sorry for uncovinience again and again.
Take it as is.

[Dreamsorcerer]

My application has broken, too. I would expect this kind of change in 4.0, according to semver best practices. I have nothing against the idea, but I am against backward-incompatible changes within the same major version.

Yes, if I had realised there were several users actually using this behaviour, I wouldn't have backported it. Though, I can also argue that no high-level, documented behaviour has changed, you were depending on low-level implementation details for your app.

I am using asyncio.ensure_future that works.

Which, again, is a low-level function. The documentation suggests you use the high-level asyncio.create_task(). Again, if you consider run_app() to be a high-level function similar to asyncio.run(), then it is the equivalent of writing:

asyncio.ensure_future(something())
asyncio.run(my_app())

Which will also be broken.

[webknjaz]

I agree with Sam: this is not a breaking change API-wise and it doesn't go against SemVer. SemVer operates the term "public API" which usually means a combination of function arguments and the return values — a contract, in the words. The implementation is a private thing and relying on the pre-existing loop was an internal detail, a side-effect. We are at mercy of the Hyrum's law¹ here.

This side-effect never was a part of the public API, we never declared that this quirk was supported, we never encouraged the users to rely on it. In fact, this is something that we would rather explicitly discourage as a design antipattern because of how much hard-to-debug harm it may bring.

I think, though, we could improve the documentation to mention the recommendation to avoid such a design flaw.

Footnotes

1. TL;DR It's when the consumers depend on the implementation details and side-effects, and then demand official support for those — https://www.hyrumslaw.com/ ↩

[vmarkovtsev]

Shall I point at a hundred other "undocumented internal details" that everybody relies on implicitly? Your excuses are weird @webknjaz @Dreamsorcerer The words are fancy, but the impact is the same: you broke the workflow of many users, and new reports keep referencing this pull request.

Anyway, the fix is very easy, so no drama. Very far from cosplaying GNOME developers.

[webknjaz]

This has turned into another maintainer-blaming thread. Locking it to stop these insults. We've admitted that it was an oversight and provided an explanation of why this needs to remain as is. I'm not accepting any more complaints from people who don't actually care enough to take part in the ongoing development of aiohttp and only come here to complain — repeating the same thing over and over again is not constructive.

With the behavior of complainers not piling up, this feels like a personal attack towards the maintainers who've actually spent countless hours maintaining this project. So I'm reminding you that we have a CoC. We welcome anyone to participate in constructive discussion but don't cross the line.

P.S. The only action item I see currently is improving the docs. If anybody is up to it — feel free to send a PR.

[Dreamsorcerer]

For anybody that has encountered this problem, we've updated the documentation with the above recommendations: https://docs.aiohttp.org/en/stable/web_advanced.html#complex-applications

While the loop parameter provided a quick way to get an application working again, this was mainly included due to some internal testing code and is not recommended for public use. If you continue using your own loop with run_app() I fully expect you to encounter issues again, so please take the time to update your code.

As for a low-level API, it looks like this is already covered by app runners. So, if you insist on using low-level functions and handling your own event loop, then use the app runners, not the high-level run_app() function. https://docs.aiohttp.org/en/stable/web_advanced.html#application-runners