search

dmontagu / fastapi-utils

Reusable utilities for FastAPI
MIT License
1.84k stars 163 forks source link

fix(tasks): Fix @repeat_every blocking app startup #308

Closed Keiishu closed 1 month ago

Keiishu commented 2 months ago

Description of the bug

A function using @repeat_every blocks app startup on version 0.6.0 when it is called in the context manager. This should fix #305.

Description of the changes

Replacing (https://github.com/dmontagu/fastapi-utils/blob/master/fastapi_utils/tasks.py#L77) with asyncio.ensure_future(loop()) works. Every test passes for me.

ollz272 commented 2 months ago

@dmontagu @yuval9313 please can this be looked at asap

Keiishu commented 2 months ago

I'm not sure why those tests don't pass, I did run pytest locally and I got no issues whatsoever. Should I have tried with make test instead? I'll investigate.

Keiishu commented 2 months ago

My bad, I don't know what happened with my tests then, but they didn't pass locally neither in the end. They do now ; it was just a missing await.

EDIT: I'm sorry, we're back at the beginning. It blocks FastAPI's context manager. Looking at Starlette's code (https://github.com/encode/starlette/blob/9f16bf5c25e126200701f6e04330864f4a91a898/starlette/routing.py#L732), it seems the issue stems in the fact that Starlette is waiting for the end of the lifespan, while here, by awaiting the loop, we are never getting out of it. It needs to be a coroutine, but then the tests fail...

Keiishu commented 1 month ago

For now, I'll revert my second commit (the await), keep this PR as a draft and wait for further insight on what's happening with the tests.

mgkid3310 commented 1 month ago

I've reviewed the test_tasks.py code and I guess there's a problem with how repeat_every() is handled in pytest. For example, while test for repeat counter fails as the counter stays at 0 but it expects 3 (max_repetitions) meaning that the iterations didn't work as intended, when I increase the sleep seconds to 5 (that makes total execution time 15 seconds) the test still completes in 0.11s.

So my guess is: the loop() function and the func() (which is the TestRepeatEveryBase.increase_counter()) is not executed at all before the assert self.counter == max_repetitions evaluation. It is true that await increase_counter_task() is valid statement, but since loop() is ensured in the future, we need a different way for assert self.counter == max_repetitions to wait loop() to complete the func() calls.

mgkid3310 commented 1 month ago

I'm working on adding a on_complete parameter to repeat_every() and using that to trigger an event once the loop is complete, not sure if it will work but for now I guess it's the only option I see.

mgkid3310 commented 1 month ago

310 I've merged your commits and added my further fixes for tests. I've solved problems for most tests (4 out of 6 failing tests) but failed to solve the remaining 2 before going to bed. Can you take a look at the code?

yuval9313 commented 1 month ago

Hey, this is still marked with a draft, would you like this PR to be reviewed or the other one?

EDIT: I prefer the #310 so I'm going to focus on merging it

Keiishu commented 1 month ago

No problem @yuval9313 ; #310 is a fork of this PR with updated tests, so we should focus there. I'll close this one.

Keiishu commented 1 month ago

310 I've merged your commits and added my further fixes for tests. I've solved problems for most tests (4 out of 6 failing tests) but failed to solve the remaining 2 before going to bed. Can you take a look at the code?

Nice, thanks! I really can't focus on this right now, until Thursday at least, so I'll take a look at it then if nobody fixed it by then.