Skip to content
This repository was archived by the owner on May 31, 2021. It is now read-only.

Improve consume/produce examples #10

Merged

Conversation

sylvainb
Copy link
Contributor

@sylvainb sylvainb commented Oct 16, 2016

Use asyncio.gather instead of asyncio.ensure_future in the first example (more understandable). Add comments to the second example.

The asyncio.ensure_future is not introduced in the previous examples.
@@ -32,6 +32,7 @@

loop = asyncio.get_event_loop()
queue = asyncio.Queue(loop=loop)
asyncio.ensure_future(produce(queue, 10), loop=loop)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it is deliberate to not wait for the completion of the producer.

Copy link
Collaborator

@vxgmichel vxgmichel Oct 17, 2016

Choose a reason for hiding this comment

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

Yes, the idea here is to run the producer in a background task while waiting for the consumer to complete. However, I'm not opposed to this change if you think it makes the example more understandable. As an alternative, it is possible to make the producer task more explicit:

loop = asyncio.get_event_loop()
queue = asyncio.Queue(loop=loop)
producer_task = loop.create_task(produce(queue, 10))
loop.run_until_complete(consume(queue))
assert producer_task.done()
loop.close()

In any case, it's probably good to discourage the fire and forget approach for the background task.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yesterday, during AsyncIO workshop at PyCON-FR, it was hard for two attendees to understand why we have ensure_future for one coroutine, and not for the other.

I've proposed to change the example as proposed by @sylvainb, it seems more understandable by available beginners.
I've also suggested to propose a pull request for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then it's fine with me, I'm merging it ;-)

@vxgmichel vxgmichel merged commit 6e8a0d6 into asyncio-docs:master Oct 17, 2016
@vxgmichel
Copy link
Collaborator

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants