[issue37658] In some cases asyncio.wait_for can lead to socket leak.

2021-05-14 Thread nmatravolgyi


nmatravolgyi  added the comment:

I did a little research on how the `wait_for` could be improved. There are four 
alternate implementations you can find here: 
https://github.com/Traktormaster/wait-for2/blob/issue37658/wait_for2/impl.py

The repository (on the linked branch) has tox set-up and a test that asserts 
the behaviour of builtin and alternate implementations.

Quick summary:
  - wait_for (builtin): either leaks resources or ignores cancellation
  - wait_for (preferred alt): works as expected (I'm going to discuss this 
below)
  - wait_for_special_raise: works, but has pitfalls
  - wait_for_no_waiter*: These were proposed by @aaliddell on the related PR: 
https://github.com/python/cpython/pull/26097#issuecomment-840497455 I could not 
make them work, but I might be missing something. I think the fact that the 
inner coroutine gets wrapped into a Future introduces the race-condition, but I 
don't know how to test that. The general idea of this would be the best if an 
implementation was possible.


About the actually working alternate implementation I made:
In my opinion there is no way to implicitly handle losing a result properly in 
case of a cancellation, since it arbitrarily breaks the flow of execution. I'd 
look into having `wait_for(...)` support cleanup callbacks when a cancellation 
and completion happens at the same time. Something like:

```python

async def create_something():
# this acquires some resource that needs explicit cleanup after some work
return object()

def cleanup_something(inst):
inst.close()

t = asyncio.ensure_future(create_something())
x = await asyncio.wait_for(t, timeout=10, cancel_handler=cleanup_something)
try:
pass  # normal work with x
finally:
cleanup_something(x)  # cleanup at normal execution
```

The inner task is still responsible for handling the resource before it 
returns, which means if the inner task is cancelled, there must be no leak if 
the implementation is correct. If no cancellation happens, everything is 
"fine". Finally, the waiter task would be able to handle its cancellation and 
the simultaneous completion of the inner task if the caller code provides a 
callback.

Unfortunately this requires the user of `wait_for` to be aware of this 
race-condition. However it makes it possible to be handled properly when the 
waited future's result requires a cleanup.

--
nosy: +nmatravolgyi

___
Python tracker 
<https://bugs.python.org/issue37658>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue43389] Cancellation ignored by asyncio.wait_for can hang application

2021-05-09 Thread nmatravolgyi


Change by nmatravolgyi :


--
stage:  -> resolved
status: open -> closed

___
Python tracker 
<https://bugs.python.org/issue43389>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue42130] AsyncIO's wait_for can hide cancellation in a rare race condition

2021-05-09 Thread nmatravolgyi


nmatravolgyi  added the comment:

I've also found this deficiency of asyncio.wait_for by debugging an obscure 
hang in an application. Back then I've quickly made an issue about it: 
https://bugs.python.org/issue43389

I've just closed it as duplicate, since this issue covers the same bug and has 
been around longer.

I'm surprised this issue has not got more attention. This definitely needs a 
fix. Ignoring a CancellationError is something that is heavily discouraged by 
the documentation in general. Silently ignoring/eating a cancellation makes 
this construct unreliable without good workarounds, aside from not using it.

For a specific application, this was so broken, that I had to come up with a 
fix in the short term. I made an asyncio.wait_for variant available as a 
library that fixes the problem: https://github.com/Traktormaster/wait-for2

The repository has a detailed description of the issue and the way it fixes it. 
It also has test cases to assert the behaviour of the builtin and the fixed 
wait_for construct from the library.

--
nosy: +nmatravolgyi

___
Python tracker 
<https://bugs.python.org/issue42130>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue43389] Cancellation ignored by asyncio.wait_for can hang application

2021-05-09 Thread nmatravolgyi


nmatravolgyi  added the comment:

Closing as duplicate, because there was already an issue for this bug: 
https://bugs.python.org/issue42130

--
resolution:  -> duplicate

___
Python tracker 
<https://bugs.python.org/issue43389>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue43389] Cancellation ignored by asyncio.wait_for can hang application

2021-03-03 Thread nmatravolgyi


nmatravolgyi  added the comment:

One more thing. I've figured out that I can fix the cancellation around the 
asyncio.wait_for() with asyncio.shield() like:

try:
await asyncio.shield(wf := 
asyncio.ensure_future(asyncio.wait_for(self.event.wait(), timeout=60.0)))
except asyncio.CancelledError:
wf.cancel()
result = await asyncio.gather(wf, return_exceptions=True)
# here I know there is a cancellation AND I might have a result as well!
raise

However I don't like the idea of writing all that boilerplate for every 
wait_for usage. I still might be overlooking something, but at least I have 
adequate workarounds.

I'm curious what the consensus will be on this issue. I'm certain it should be 
documented though. Right now there is no mention of ignoring/eating a 
cancellation.

--

___
Python tracker 
<https://bugs.python.org/issue43389>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue43389] Cancellation ignored by asyncio.wait_for can hang application

2021-03-03 Thread nmatravolgyi


nmatravolgyi  added the comment:

I've quickly wanted to create a suitable solution for myself. I made a small 
library with a asyncio.wait_for()-like function using asyncio.wait(). The 
prototype worked, so I put together a small project. When I ran tox and 
realized that this issue with wait_for is only present on py38 and py39 
(possibly py310). The wait_for does not get stuck with py36, py37 and pypy3.

The repo is a little bare bones, but you can run tox after checkout: 
https://github.com/Traktormaster/wait-for2

Right now the tests are set-up that they expect wait_for to get stuck so only 
py38 and py39 passes.

I'm pretty sure the side-effect of returning the future's result when handling 
cancellation is not desired. However I'm not sure how to handle it correctly. 
The repo holds a demo of what I suggested in the beginning of this thread 
(CancelledWithResultError). It works but it is limited.

--

___
Python tracker 
<https://bugs.python.org/issue43389>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com



[issue43389] Cancellation ignored by asyncio.wait_for can hang application

2021-03-03 Thread nmatravolgyi


New submission from nmatravolgyi :

I have found myself debugging a *very* not intuitive behavior regarding 
asyncio.wait_for that I'd consider a bug/deficiency. The problem very simply 
put: wait_for will return the wrapped future's result even when it is being 
cancelled, ignoring the cancellation as it has never existed.

This will make parallel execution-waits hang forever if some simple conditions 
are met. From the perspective of this snippet every task must exit so it just 
needs to wait. I know cancellation *can* be ignored, but it is discouraged by 
the documentation for this reason exactly.

tasks = [...]
for t in tasks:
t.cancel()
results = await asyncio.gather(*tasks, return_exceptions=True)

I already know that this behavior has been chosen because otherwise the 
returned value would be lost. But for many applications, losing an explicit 
cancellation error/event is just as bad.

The reason why ignoring the cancellation is critical is because the cancelling 
(arbiter) task cannot reliably solve it. In most cases having repeated 
cancellations in a polling wait can solve this, but it is ugly and does not 
work if the original wait_for construct is in a loop and will always ignore the 
cancellation.

The most sensible solution would be to allow the user to handle both the return 
value and the cancellation if they do happen at once. This can be done by 
subclassing the CancelledError as CancelledWithResultError and raising that 
instead. If the user code does not handle that exception specifically then the 
user "chose" to ignore the result. Even if this is not intuitive, it would give 
the user the control over what really is happening. Right now, the user cannot 
prefer to handle the cancellation or both.

Lastly, I may have overlooked something trivial to make this work well. Right 
now I'm considering replacing all of the asyncio.wait_for constructs with 
asyncio.wait constructs. I can fully control all tasks and cancellations with 
that. I've made a simple demonstration of my problem, maybe someone can shed 
some light onto it.

--
components: asyncio
files: aio_wait_for_me.py
messages: 388032
nosy: asvetlov, nmatravolgyi, yselivanov
priority: normal
severity: normal
status: open
title: Cancellation ignored by asyncio.wait_for can hang application
type: behavior
versions: Python 3.8, Python 3.9
Added file: https://bugs.python.org/file49849/aio_wait_for_me.py

___
Python tracker 
<https://bugs.python.org/issue43389>
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com