Fail test if there are unhandled exceptions or pending tasks.

[Kentzo]

This is a backport from @GreatFruitOmsk's internal async_unittest written by @amezin.

[Martiusweb]

Hi, this is a nice feature. I think that the best way to integrate it in asynctest is to use it through the @fail_on decorator:
http://asynctest.readthedocs.io/en/latest/asynctest.case.html#asynctest.case.asynctest.fail_on

That would allow users to deactivate this check when they need to: for instance, when a user overrides the default exception handler to centralize the way common exceptions are handled in its asyncio app.

If you want to update your PR and need some help doing so, don't hesitate to ask for help. I added some documentation in the _fail_on module.

[Kentzo]

@Martiusweb For this feature it's important to call gc.collect() after event loop is closed.

The _tearDown method is called to soon for that. What would you suggest?

[RemiCardona]

My experience with gc tells me that adding a call to collect() is a bad idea.

First of all, I think this would be quite unexpected by a lot of users. Not to mention the potential effect of repeatedly calling this function on performance.

Second of all, relying on collect() feels inherently wrong to me, as the GC is an implementation detail of CPython.

Maybe there's a way to track the futures and tasks coming out of the event loop and check them at the end of the test? Subclassing? Just thinking "out loud" really.

My 2¢

Rémi

[Kentzo]

I cannot imagine a test that would suffer more than a couple ms from gc.collect, but that can be a made a separate configuration option. But I believe that it should be on by default, because you do want errors like this to be fixed asap. And due to their unpredictable nature the only way is to “randomly” call gc.collect along the execution. The only reliable source is to cause Task’s `__del__` to be triggered. I guess if there would be a better way, asyncio would already support it. It worths to take a look at how other major asyncio frameworks (e.g. aiohttp) test against that though. There was a thread on python ideas not long ago about adding gc.collect to unittest. I’ll attach a link if I find it. Best Regards Ilya Kulakov

…

On Dec 8, 2017, at 5:37 AM, Rémi Cardona ***@***.***> wrote: My experience with gc tells me that adding a call to collect() is a bad idea. First of all, I think this would be quite unexpected by a lot of users. Not to mention the potential effect of repeatedly calling this function on performance. Second of all, relying on collect() feels inherently wrong to me, as the GC is an implementation detail of CPython. Maybe there's a way to track the futures and tasks coming out of the event loop and check them at the end of the test? Subclassing? Just thinking "out loud" really. My 2¢ Rémi — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[Kentzo]

On the second thought, it can be removed and users can run it manually on demand.

What has to be fixed is an ability to run checks after the loop is closed and destroyed.

[Martiusweb]

Hi, I'm quite busy this month, and I can't find enough time to correctly review the opened PRs, thus, sorry for the delays.

If you think that we should rework @fail_on to be able to perform some checks at a different time, I'm OK with this. If you need to ensure that the loop has been destroyed, I think it's doable with a finalizer (https://docs.python.org/3/library/weakref.html#weakref.finalize). However, the checks must always be triggered inside the test case, which means that we probably need to ensure that there are no other references to the loop in user's code... which is a pretty big assumption. On the other hand, I'm not sure that we really need this (ensure that the loop is destroyed).

[Kentzo]

@Martiusweb Destruction is necessary to trigger the check. We could issue a warning if loop is still alive by the end of the test.

[Kentzo]

@Martiusweb Any thoughts regarding that improvement?

[Martiusweb]

I really think this should be implemented with @fail_on:

That would allow users to deactivate this check when they need to: for instance, when a user overrides the default exception handler to centralize the way common exceptions are handled in its asyncio app.

I also have concerns with the use of gc.collect(): how does collecting a pending task raises an exception? I don't think we should rely on this.

Maybe this PR should be split in two:

• detecting unhandled exceptions using fail_on by overriding the exception handler (as done in this PR)
• an other fail_on check detecting pending tasks by looking at what's left in asyncio.Task.all_tasks()

[Kentzo]

I also have concerns with the use of gc.collect(): how does collecting a pending task raises an exception? I don't think we should rely on this.

These are details of CPython's asyncio implementation: The error is only triggered upon object's destruction with a pending exception.

Alternatively one could go over all the tasks and check for the _exception attribute, but that seems to rely even more on the implementation detail compared to mere object's lifetime.

Perhaps @1st1 or @vstinner could give us an advice.

[vstinner]

IMHO it's a bug in asyncio which should not keep live frames. But nobody came with a fix for this. There is now traceback.clear_frames().

[1st1]

I'd just iterate over all tasks using asyncio.all_tasks(). Check if task is done or not; if not - wait for it; if yes, check if it has failed.

[Kentzo]

Not sure I understood @vstinner comment.

We are trying to come up with a solution to check if there are tasks with pending exceptions by the time the test is done.

One way is to rely on CPython’s implementation that calls loop’s unhandeled exception handler upon Task’s deletion if it has a pending exception.

[Kentzo]

@1st1 We do not control the flow within a test case. Tasks with unhandled exceptions can be deleted before the execution will reach our code. These deleted tasks won't appear in all_tasks.

[1st1]

Another idea would be to catch all warnings, match them, and turn them into an error if they're coming from an asyncio task.

[Kentzo]

But again, warnings are issued upon object's destruction which may or may not happen and therefore usage of gc.collect is inevitable.

[1st1]

Well, yes, set up a custom warnings filter, and run gc.collect() after the test; record any event of logging a warning in Task.__del__, fail the test if that happens.

[Kentzo]

@1st1 Pushed one more attempt of an implementation that avoids gc.collect.

One issue though is that context passed to custom exception handler does not receive source_traceback. Neither Task._log_traceback is set.

[Martiusweb]

I don't understand what can be captured via warnings.

The first solution we're exploring is relying on a custom exception handler in the loop and relying on gc.collect() to ensure the exception handler is called. I'm concerned about the portability and sustainability of this solution because I don't know if we can rely on gc.collect() on other implementations of python (in particular pypy). I also don't know if we should assume this behavior in other implementations of asyncio (like uvloop).

In fact, asynctest already assumes a lot about internals of the implementation of the loop, which is something I'd like to take care of (one day...).

Alternatively, we can create a wrapper around the task to track pending exceptions. Asynctest would hook itself at a higher level, which seems safer. The main downside I see is that it's only available since 3.4.4 (we can test if the feature is available and raise a warning if not).