Skip to content

asyncio integration#1671

Merged
antonpirker merged 25 commits intomasterfrom
antonpirker/asyncio-integration
Oct 17, 2022
Merged

asyncio integration#1671
antonpirker merged 25 commits intomasterfrom
antonpirker/asyncio-integration

Conversation

@antonpirker
Copy link
Contributor

@antonpirker antonpirker commented Oct 10, 2022

Make sure each asyncio task that is run has its own Hub and also creates a span.

Fixes #1333
Fixes #772

@antonpirker antonpirker marked this pull request as ready for review October 12, 2022 11:18
@antonpirker antonpirker enabled auto-merge (squash) October 14, 2022 11:21
# WARNING:
# If the default behavior of the task creation in asyncio changes,
# this will break!
task = Task(_coro_creating_hub_and_span(), loop=loop)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be

Suggested change
task = Task(_coro_creating_hub_and_span(), loop=loop)
task = Task(_coro_creating_hub_and_span, loop=loop)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but how did it work before lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it work now, actually 😄 The correct version is the first one.

In [1]: from asyncio import Task

In [2]: async def foo():
   ...:     print("test")
   ...: 

In [3]: await Task(foo)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [3], in <cell line: 1>()
----> 1 await Task(foo)

TypeError: a coroutine was expected, got <function foo at 0x7f7fb9241f30>

In [4]: await Task(foo())
test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, please test all cases @antonpirker
this is what cpython does
https://github.com/python/cpython/blob/main/Lib/asyncio/base_events.py#L436-L444

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good catch @vmarkovtsev!
This lead to this fix: #1689

@antonpirker antonpirker merged commit 973b2f6 into master Oct 17, 2022
@antonpirker antonpirker deleted the antonpirker/asyncio-integration branch October 17, 2022 14:59
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

Successfully merging this pull request may close these issues.

Tracing: Some concurrently created Spans are not recorded. asyncio.gather tracing produces nested spans

4 participants