Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

asyncio task cancellation can be missed by wait_for_aio() #128

Open
flukeborder opened this issue Feb 8, 2024 · 0 comments
Open

asyncio task cancellation can be missed by wait_for_aio() #128

flukeborder opened this issue Feb 8, 2024 · 0 comments

Comments

@flukeborder
Copy link

I have a program with an asynchronous task which internally uses Socket.aread(). I have found that such a task might at times fail to get cancelled (via asyncio Task.cancel()), that is, asyncio.CancelledError does not get raised to the upper-level task.

I believe I have narrowed the problem down to _aio.asyncio_helper.wait_for_aio(). This coroutine awaits on a future which gets cleared once the underlying NNG asynchronous task is completed. asyncio.CancelledError is captured and the underlying NNG asynchronous task is cancelled, should the asyncio task cancellation occur during the await (lines 69-74).
Subsequently, a check is made whether the NNG asynchronous task has been cancelled, based on nng_aio_result. If so, asyncio.CancelledError is raised to the caller (lines 77-79).

There is however a possibility that the underlying NNG asynchronous task might complete after the asyncio cancellation (asyncio.CancelledError raised during the await), but before the call to lib.nng_aio_cancel() is made in line 73. According to the documentation of nng_aio_cancel:

If no operation is currently in progress (either because it has already finished, or no operation has been started yet), then this function has no effect.

Therefore, in such a scenario, nng_aio_result in line 77 will not return NNG_ECANCELED, and asyncio.CancelledError will not be raised to the caller - which I believe is a bug.

A possible solution would be to set an additional flag in the except block in line 71 and include this flag in the test made in line 78 when deciding whether asyncio.CancelledError shall be raised to the caller. Relying solely on the return value of nng_aio_result is not sufficient.

I arrived at these conclusions by debugging down to the pynng boundary and then by static analysis - but the problem seems pretty obvious. I have not analysed the code for the other asynchronous backends, which might be affected by a similar problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant