#11841 fix pending futures #11890

stalkerg · 2023-07-01T07:30:09Z

It should fix #11841.

The main problem if corouting.send returns future in pending status which means we must back to the event loop and wait until it will be resolved. Otherwise, we will see the error "await wasn't used with future" which comes from the send code: https://github.com/python/cpython/blob/3.11/Modules/_asynciomodule.c#L1609 .

We are doing the same thing if we receive Deferred but for some reason ignored asyncio futures. This PR does the same thing for pending futures as for Deferred.

Most problems come from asyncio.ensure_future function, which used by aiohttp. Without these changes not possible to use aiohttp inside Twisted even if you are using asyncio reactor and it reduces compatibility with tons of libraries.

for more information, see https://pre-commit.ci

… into support-pending-feature

stalkerg · 2023-07-01T08:18:46Z

please review

adiroiban

Many thanks for the changes.

I don't have experience with future and it looks like the automated tests is not executed... so I can't do a proper review.

I left a few general comments regarding the PR.

I think the main change looks good... but I think that this PR needs improvement on documenting the changes and the tests

Thanks again

adiroiban · 2023-07-01T13:19:39Z

src/twisted/test/test_defer.py

+    @ensuringDeferred
+    async def test_fromCoroutineWithEnsureFuture(self) -> None:
+        """
+        L{Deferred.fromCoroutine} should properly process pending futures


What does "properly" means ?

I don't know why this has no coverage.
Is this test executed?

What does "properly" means ?

It means we should back control the event loop before we can call send again. Not sure how to describe it.
And yes, this test is executed, at least locally, and without these changes is failed.

It means we should back control the event loop before we can call send again.

Then the docstring should say those words, rather than "properly" :-)

See https://jml.io/test-docstrings/

@glyph thanks! Good page.

adiroiban · 2023-07-01T13:24:55Z

src/twisted/newsfragments/11841.bugfix

@@ -0,0 +1 @@
+`defer.Deferred.fromCoroutine` now correctly processes pending futures.


By reading the code, it looks like any @defer.inlineCallbacks is affected by this change.

I don't have any experience with futures and coroutines... so maybe "correctly" makes sense for someone who has experience.

But I think that it would help add more information about what is considered "correctly"

Okey, I will explain it in more detail.

stalkerg · 2023-07-01T13:56:49Z

@adiroiban, thanks for the review!

I don't have experience with future and it looks like the automated tests is not executed... so I can't do a proper review.

It's triggered locally, do you have any idea why it's not working in CI?

I think the main change looks good... but I think that this PR needs improvement on documenting the changes and the tests

Sure, I will try to improve it. Also, who can look into this PR with futures and coroutines experience?

stalkerg · 2023-07-01T14:32:37Z

A little more background for this issue.
Deferred.fromCoroutine is not entirely correct. It's even described in the documentation:

Coroutine functions return a coroutine object, similar to how generators work.

courutine.send in the asyncio it return awaitable object, which includes __await__. It's not a coroutine automatically and can be Future which is close to Derefer and should be scheduled separately on the event loop if it's not already resolved(done).

Basically, the original Deferred.fromCoroutine supported only trivial coroutines without Future or Task inside.

stalkerg · 2023-07-01T15:02:26Z

I don't have experience with future and it looks like the automated tests is not executed... so I can't do a proper review.

I can reproduce it locally, working on it.

graingert · 2023-07-01T15:08:25Z

Deferred.fromCoroutine intentionally does not support asyncio coroutines they have different cancellation semantics etc. To wait on a future you should explicitly call fromFuture(asyncio.create_task)

graingert · 2023-07-01T15:08:44Z

Deferred.fromCoroutine intentionally does not support asyncio coroutines they have different cancellation semantics etc. To wait on asyncio coroutines you should explicitly call fromFuture(asyncio.create_task)

stalkerg · 2023-07-01T16:27:38Z

@graingert

Deferred.fromCoroutine intentionally does not support asyncio coroutines

I see, you support async/await but without asyncio. But it's very confusing in docs, https://docs.twisted.org/en/stable/core/howto/defer-intro.html#inline-callbacks-using-yield here we have the phrase:

Coroutines are supported by dedicated Python syntax, are compatible with asyncio, and provide higher performance.

also, the documentation includes examples only with fromCoroutine and has no mention fromFuture.
PS I think nowadays async/await in Python is associated with asyncio (or trio).

exarkun · 2023-07-01T20:01:28Z

PS I think nowadays async/await in Python is associated with asyncio (or trio).

This doesn't mean that only asyncio and trio are allowed to use them, nor that everyone using them is using asyncio or trio.

stalkerg · 2023-07-02T03:05:21Z

This doesn't mean that only asyncio and trio are allowed to use them, nor that everyone using them is using asyncio or trio.

Sure, but it's just in the context of confusion. Also, because we are using asyncio reactor, developers can have expectations of compatibility.

Maybe we should raise an exception if during follow coroutines we found Future? Because current errors like await wasn't used with future doesn't explain anything (and it's come from C code). Also because sometimes not easy to check what exactly the async function uses deep inside.

but still, my PR is working, I know it seems like will not be correct during the cancel case, but maybe is it possible to handle it?

glyph · 2023-07-02T03:55:20Z

Without these changes not possible to use aiohttp inside Twisted even if you are using asyncio reactor and it reduces compatibility with tons of libraries.

That is not accurate. Deferred.fromFuture and Deferred.asFuture should do any conversion from/to aiohttp's async primitives. If we can do this automatically and losslessly for the user then by all means we should, but I'd much rather require manual conversion all the time than have some programs randomly fail to run if you selected the wrong reactor.

I haven't evaluated the code here yet though, maybe it's great :-)

stalkerg · 2023-07-02T03:58:45Z

@glyph

That is not accurate. Deferred.fromFuture and Deferred.asFuture should do any conversion from/to aiohttp's async primitives.

Deferred.fromFuture is working, but I wrongly expected Deferred.fromCoroutine also should work (because coroutine deep inside can use Future or Task)

glyph · 2023-07-02T04:03:10Z

Ah. Yes, the naming here is unfortunate. Clearly at least the documentation could use an improvement here.

stalkerg · 2023-07-03T01:22:42Z

@exarkun, if I will try to solve cancel semantics difference, does this PR have sense for you? If not, I will close it.

exarkun · 2023-07-03T11:40:14Z

@exarkun, if I will try to solve cancel semantics difference, does this PR have sense for you? If not, I will close it.

I'll leave this to someone else to answer, I think.

graingert · 2023-07-03T11:48:05Z

It's also asyncio.current_task() that needs to work too

stalkerg and others added 4 commits July 1, 2023 16:16

fix pending futures twisted#11841

a47f305

[pre-commit.ci] auto fixes from pre-commit.com hooks

c1c0cde

for more information, see https://pre-commit.ci

fix mypy issues

e50a0ec

Merge branch 'support-pending-feature' of github.com:stalkerg/twisted…

a4f6091

… into support-pending-feature

stalkerg mentioned this pull request Jul 1, 2023

RuntimeError: await wasn't used with future aio-libs/aiohttp#7117

Closed

1 task

add newsfragment

837dda2

chevah-robot added the needs-review label Jul 1, 2023

chevah-robot requested a review from a team July 1, 2023 08:18

adiroiban reviewed Jul 1, 2023

View reviewed changes

adiroiban requested a review from a team July 1, 2023 13:26

exarkun mentioned this pull request Jul 1, 2023

asyncio.ensure_future not working in reactor with inlineCallbacks #11841

Open

luislhl mentioned this pull request Feb 7, 2024

fix: health endpoint when using --x-asyncio-reactor HathorNetwork/hathor-core#940

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#11841 fix pending futures #11890

#11841 fix pending futures #11890

stalkerg commented Jul 1, 2023

stalkerg commented Jul 1, 2023

adiroiban left a comment •

edited

adiroiban Jul 1, 2023

stalkerg Jul 1, 2023

glyph Jul 2, 2023

stalkerg Jul 2, 2023

adiroiban Jul 1, 2023

stalkerg Jul 1, 2023

stalkerg commented Jul 1, 2023

stalkerg commented Jul 1, 2023

stalkerg commented Jul 1, 2023

graingert commented Jul 1, 2023

graingert commented Jul 1, 2023

stalkerg commented Jul 1, 2023

exarkun commented Jul 1, 2023

stalkerg commented Jul 2, 2023 •

edited

glyph commented Jul 2, 2023

stalkerg commented Jul 2, 2023

glyph commented Jul 2, 2023

stalkerg commented Jul 3, 2023

exarkun commented Jul 3, 2023

graingert commented Jul 3, 2023

		@@ -0,0 +1 @@
		`defer.Deferred.fromCoroutine` now correctly processes pending futures.

#11841 fix pending futures #11890

Are you sure you want to change the base?

#11841 fix pending futures #11890

Conversation

stalkerg commented Jul 1, 2023

stalkerg commented Jul 1, 2023

adiroiban left a comment • edited

Choose a reason for hiding this comment

adiroiban Jul 1, 2023

Choose a reason for hiding this comment

stalkerg Jul 1, 2023

Choose a reason for hiding this comment

glyph Jul 2, 2023

Choose a reason for hiding this comment

stalkerg Jul 2, 2023

Choose a reason for hiding this comment

adiroiban Jul 1, 2023

Choose a reason for hiding this comment

stalkerg Jul 1, 2023

Choose a reason for hiding this comment

stalkerg commented Jul 1, 2023

stalkerg commented Jul 1, 2023

stalkerg commented Jul 1, 2023

graingert commented Jul 1, 2023

graingert commented Jul 1, 2023

stalkerg commented Jul 1, 2023

exarkun commented Jul 1, 2023

stalkerg commented Jul 2, 2023 • edited

glyph commented Jul 2, 2023

stalkerg commented Jul 2, 2023

glyph commented Jul 2, 2023

stalkerg commented Jul 3, 2023

exarkun commented Jul 3, 2023

graingert commented Jul 3, 2023

adiroiban left a comment •

edited

stalkerg commented Jul 2, 2023 •

edited