Revert "Avoid ignoring asyncio exceptions in coroutines" #672
Conversation
This reverts commit f7552be.
|
Can one of the admins verify this patch? |
7 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Hey @simskij, sorry you're having trouble with that, though I'm surprised to hear that exceptions raised in the asyncio tasks that are internal to pylibjuju (and internal to asyncio/websocket connections) actually crashing your tests. Do your tests pass %100 after this revert change? |
Verified, yes. Before that, the stack trace looks like: That |
|
@rbarry82 could you provide us with some steps to reproduce this? We would like to discard other potential errors? |
|
Hey @rbarry82, thanks for the explanation, @juanmanuel-tirado told me that this is blocking you, sorry to hear that, let's figure this out and help you guys asap. So your explanation doesn't make sense to me for several reasons. First of all, what you see here is not an actual error (like, return 1), but just a printout of an internal error on stderr. And the f7552be you mentioned is about handling the exception when the tasks are destroyed, meaning those exceptions we see here are surfaced long after whatever was running is finished, so it shouldn't effect on the result of any tests whatsoever.
I don't fully understand what you mean by "OSError moves onto 916", and "there's a loop with debug". It would also help if you could elaborate on that. In any case, we'd be fine to land this to help you guys out if this is absolutely needed asap, but I suspect that whatever is going on in your tests is misdiagnosed to be related to this and you'll still face it even if this revert is landed. We'll be paying close attention to this to help you folks unblock asap 👍 |
|
This happens every single time a model is destroyed with What we see here is the exception thrown by Python when you try to do anything to a closed socket. Sorry for using bare line numbers. I mean that, in This is just plain Honestly, this is trivially reproducible anyway. 💣 💣 First you'll see a |
|
Hey @rbarry82, thanks for the example, this is exactly what I was talking about. First, I agree with you, that it looks like the attempt to reduce the background noise (f7552be) is not doing a proper job, as a result, we're getting a bunch of logs about the Task errors. However, in your example, you are getting a Regardless, I agree with you for the first reason I mentioned, that we should land this PR and revert that change, as it appears to be not doing the job well. So I'm gonna go ahead and give this a 👍 . However, I still doubt that reverting this will stop you from getting an error, or those Task exception logs. I hope I'm wrong, but let's at least unblock you guys. If things persist, feel free to open an issue and we'll go from there 👍 |
|
It does, but please see here, which comes from this fixture calling That throws the websocket connection exception back at
async def close(self, to_reconnect=False):
...
active_tasks = [t for t in self._pinger_task, self._receiver_task, self._debug_log_task if t]
map(lambda t: t.cancel(), active_tasks)
for t in active_tasks:
try:
await t
except asyncio.CancelledError:
logger.debug("Task was succesfully cancelled: %s", t) |
|
I'm not trusted to merge this, but the PR should be up-to-date now at least. 😅 |
|
|
This reverts commit f7552be. The suggested fix did not really solve the issue it was supposed to solve as it instead fails with
[Errno 9] Bad File Descriptorcrashing most of our tests.