Fix: tasks.repeat_every() and related tests

[mgkid3310]

Original Issue: #305 & #307
Original PR: #308

Purpose of PR

Since 0.6.0 any use of tasks.repeat_every() in apllication startup have halted further app processes. The cause was found to be the use of await keyword for loop() but further testing by @Keiishu have shown that the tests for tasks.repeat_every() does not allow the use of asyncio.ensure_future(), but rather results in failed tests.
This PR intends to both: 1) fix the tasks.repeat_every(); and 2) fix the related tests.

Chnages to codes

repeat_every()

• New parameter named on_complete with type NoArgsNoReturnFuncT (but compatible with async func) was added. Initial purpose was to allow tests to add a way to add event to catch when the loop has ended, but I guess this parameter can be used for other purposes in production.
• repetitions = 0 have been moved so that nonlocal keyword is not needed and some linebreaks were added for better readability.

tests/tests_tasks.py

• completed state and loop_completed() fucntion were added to test base class. This allows test cases to wait for the loop to complete by using await self.completed.wait() statement. Timeout was added to prevent function hangs, but it requires pytest-timeout package from pip to work and without it the timeout decorator will have no effect.
• General code update for fixtures to improve consistency among fixtures.
• raising_task and suppressed_exception_task are no longer staticmethod as they need self.loop_completed as parameters.
• Duplicate codes for TestRepeatEveryWithSynchronousFunction and TestRepeatEveryWithAsynchronousFunction such as increase_counter_task have been moved to TestRepeatEveryBase.

TODO

test_raise_exceptions_true test cases still result fail, so it needs to be fixed.
test_raise_exceptions_false do result pass, but I doubt if that's valid.

[mgkid3310]

@yuval9313 I've checked the previous failed builds and found they were complaining about function complexity and unsorted imports. I've all fixed and tested builds in my local machine so should be all good now.

One problem is that: the remaining two failing tests (raise_exceptions = True err catching) seems to be a tough problem. Since the current bug is blocking the app startup itself, what do you think about commenting out or removing the tests for exception raising?

[yuval9313]

@mgkid3310 I want to check the issue you are facing myself, to do so you must first finish all the test suites

EDIT:
I've attempted it myself and this prevents errors from being raised inside repeat_every due to ensure_future

Since it wasn't issue beforehand I'd accept this PR as is (with the test deleted) but I also want an issue about this since it means that exceptions inside repeat every simply won't be raised properly in-app and the app would attempt to continue either way

[mgkid3310]

@yuval9313 I've manually tested the code via:

@asynccontextmanager
async def lifespan(_):
    await test()
    yield

@repeat_every(seconds=5, raise_exceptions=True)
async def test():
    print('Hello World!')
    raise ValueError('Test error')

app = FastAPI(lifespan=lifespan)

@app.get('/', include_in_schema=False)
async def root():
    return Response(status_code=status.HTTP_200_OK)

def main():
    uvicorn.run(f'main:app', host='0.0.0.0', port=8000, log_level='info', reload=True)

if __name__ == '__main__':
    main()

INFO:     Uvicorn running on http://0.0.0.0:8000 (Press CTRL+C to quit)
INFO:     Started reloader process [28760] using WatchFiles
INFO:     Started server process [2224]
INFO:     Waiting for application startup.
Hello World!
Task exception was never retrieved
future: <Task finished name='Task-3' coro=<repeat_every.<locals>.decorator.<locals>.wrapped.<locals>.loop() done, defined at C:\Users\mgkid\Documents\GitHub\dummy_server\main.py:39> exception=ValueError('Test error')>
Traceback (most recent call last):
  File "C:\Users\mgkid\Documents\GitHub\dummy_server\main.py", line 53, in loop
    raise exc
  File "C:\Users\mgkid\Documents\GitHub\dummy_server\main.py", line 46, in loop
    await _handle_func(func)
  File "C:\Users\mgkid\Documents\GitHub\dummy_server\main.py", line 22, in _handle_func
    await func()
  File "C:\Users\mgkid\Documents\GitHub\dummy_server\main.py", line 75, in test
    raise ValueError('Test error')
ValueError: Test error
INFO:     Application startup complete.

Sure since the repeat_every() works on background the application works find regardless of the exception from the test func, the repeat does not continue.

The problem is that: 1) the test concludes itself with fail before the repeat_every() throws an exception; or 2) the exception is not compatible with the pytest environment? not in the same thread? kind of problem.

EDIT:
Reading more documents, I think in order to retrieve and handle the exception in any way (pytest for test, log in production or any purpose) the future must be awaited to be completed. Which means, if the task takes 10 sec to complete in background the test must halt for 10 sec. The problem is, to implement such behaviour the production code will have same issue and that will result in halting the FastAPI app from starting up with any kind of repeated tasks (with max_repetitions=None)

[yuval9313]

@mgkid3310, and now it seems like we found the reason why repeat_every is blocking currently, I'm OK with merging this, but just like your snippet, the app started regardless the exception

[mgkid3310]

@yuval9313 typing changed to using Union pushed

I think we're at the point where we need to think what we want from raise_exceptions. If you want a kind of safety feature that tests the function once for errors before continuing, that may not come along with wait_first because that will hold all future codes for that time. In the test code I've tried, if I try raise_exceptions=False the error is not printed on console at all. So it's:

1. Do we want the host code to catch and handle the error?
   -> host code needs to await until the repeat concludes (in error or any way, which if error does not happen can run forever)
2. Do we want to test the func() and see if it raises error?
   -> possible issue with wait_first, as the host code will have to await for that amount of time.
3. Maybe a custom function that accepts err as param in case err happens?
   -> need to change how exc is handled, plus this is still very hard (impossible in my skill) to test in pytest.

[yuval9313]

@mgkid3310 I understand, but I think it is important for every dev to handle any error might occur in the task himself, I don't mind the application crashing after a while if I put raises_exception as True

I've noticed we can check the futures .exception property

[mgkid3310]

@yuval9313 The problem is that for the host code to handle the exception in any way, the host app (FastAPI) won't start at all. Moreover, since the host code doesn't know when the exception may occur, it is quite hard to implement an event or such.

From the link you provided, it seems like the future object from ensure_future() can have exceptions set inside the loop and be checked for exceptions later. It might be worth a try.

Sure, this still won't do anything on its own, but at least it will provide a way for users to use the future object to make an event listener or so.

[yuval9313]

After consulting with some colleagues I've decided that the raise_exception is not fitting for this situation, and the way to go here is to provide a callback to allow an exception to be handled as the developer wishes.

In version V1.0 we will remove raise_exception and allow only the on_exception callback method, meanwhile, do this:

if raise_exception is callback, add the callback to the future

if raise_exception is True, log the exception if logger used and then add a warning that tells devs to migrate to on_exception

Also, dispose of the tests that makes issues, I'll add them later

[mgkid3310]

if raise_exception is callback, add the callback to the future

I'm not sure if raise_exception here is typo for on_exception or meant to be a backward compatibility thing, but if it's the latter case I'm concerned about having both boolean and callable in one variable.

Anyways, to summarize before actually coding:

1. raise_exception will be deprecated on 1.0, add notice message when activated until then.
   -> for now the logger works regardless if raise_exception is set to True or not, but only depends on if logger is not None. Do you want to keep or change this behaviour?
2. add on_exception(exc: Exception) -> bool for error handling. Return type bool is to determine if to continue the loop.
3. tests for raise_exception is to be disposed, but for the on_exception I might just implement them as I code the on_exception feature if you're ok with it.

[yuval9313]

@mgkid3310

1. Yeah, however, don't change the current logging logic, add a deprecation warning if someone adds raise_exception
2. IMO, a developer who wants to exit the loop (and close the app) should create the logic himself, we don't need to exit the loop, on_exception(exception) -> None
3. Yeah that would be great thanks!

[mgkid3310]

@yuval9313 If on_exception(exc) has no return it has no way to exit the roop gently, the only real option is to raise the excption uncatched and kill the whole loop() func. I wonder if that's what we want. I was thinking of something like

stop_flag = not await _handle_func(on_exception(exc))
-> False or None will result True for stop_flag, we'll have to document that return True will keep the loop running. The not keyword is intentional to make default None return a boolean in stop_flag.

then check for stop_flag in while statement. This way we can offer the user a way to gently continue or terminate the loop without raising exception uncatched (printing the exception in raw format on console).

[yuval9313]

I don't like it that way, it is too much of a side effect, also I don't expect devs to be cautious about the return status of their exception handlers

[mgkid3310]

@yuval9313 Understood, I'm working on the code and I'll push once the code is done.

[mgkid3310]

With the last commit both the logger (as on_exception is upward compatible with logger by having a logger inside on_exception) and raise_exceptions raises deprecation warning via the warning module (warnings.warn()), and on_exception(exc: Exception) was introduced. Tests for on_exception replaced those ones for raise_exceptions and now tests how many times the func() is called based on on_exception's behaviour (raise exception or suppress and continue).

[mgkid3310]

Found that both tests for sync and async functions were running only with sync functions, so fixed that problem so that TestRepeatEveryWithAsynchronousFunction actually runs tests for async functions (all for func, on_complete and on_exception)

[yuval9313]

Can you run poetry add --dev pytest-timeout it seems like pytest.mark.timeout is missing when running tests locally and I see no changes in poetry lock

[mgkid3310]

Although the package lock is updated, the core problem seemed to be with type hint with async functions. However when I moved over to devcontainer with python3.9, the type error does not show and the timeout package error is ignored (it was not an error which breaks the build process but just a warning). The poetry lock is fixed along with typo though.

[mgkid3310]

Turns out that the mypy didn't like conditional types such as:

func = self.increase_counter_async if is_async else self.increase_counter
return decorator(func)

where the func can be Callable[[], None] or Callable[[], Coroutine[Any, Any, None]] depending on is_async. I splitted the code flow based on is_async so that the test cases does not have any conditional types, and now the errors are fixed on my local devcontainer py3.9 with make ci-v2.