fix(tasks): Fix @repeat_every blocking app startup

[Keiishu]

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]

@dmontagu @yuval9313 please can this be looked at asap

[Keiishu]

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]

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...

[ohandyya]

@Keiishu : I think you need to remove the await. This is what was used in v0.2.1. I test your code but remove the await in my local FastAPI project, and it worked. It no longer block the app to start.

[Keiishu]

@ohandyya : That's what I did in the first commit on this PR, and it works for me, I've used it like this with no issues whatsoever in a relatively big FastAPI project. But then, tests were not passing anymore and I thought it was my fault. So I must say I'm a little bit confused.
Maybe the tests are faulty?

[mgkid3310]

I'm not familiar with sarlette but I'm pretty confident that asyncio.ensure_future doesn't return coroutine, therefore await isn't needed. I'm using the below code since the 0.6.0 repeat_every break, and this works fine for me.

def repeat(seconds: float, delay: float = None):
    def decorator(func):
        is_coroutine = asyncio.iscoroutinefunction(func)
        @wraps(func)
        async def wrapper(*args, **kwargs):
            async def repeat_function():
                if delay:
                    await asyncio.sleep(delay)

                while True:
                    if is_coroutine:
                        await func(*args, **kwargs)
                    else:
                        run_in_threadpool(func, *args, **kwargs)
                    await asyncio.sleep(seconds)

            asyncio.ensure_future(repeat_function())

        return wrapper
    return decorator

[Keiishu]

@mgkid3310 You're right, await shouldn't be necessary. But without it, the tests introduced with 0.6.0 fail. And while your code works perfectly fine for me as well, as do the first commit of this PR, the tests all fail.
So yeah, maybe there is an issue with the tests themselves...

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.

[Keiishu]

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]

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]

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]

#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]

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]

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

[Keiishu]

#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.