uasyncio/stream.py: Handle cancellation before server start. #8895
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8895 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 158 158
Lines 20939 20939
=======================================
Hits 20602 20602
Misses 337 337 |
|
Would it be better/simpler to do |
|
I'm not sure... is the fact that the scheduler uses a fifo queue and therefore you know all other ready tasks will execute once during a sleep(0) just an implementation detail or something that can be relied upon? |
The scheduler is guaranteed to be fair, so a So then it's just a question as to whether |
I guess this is part of the asyncio core... so if we do change that behavior then we can fix it. I don't mind either way, let me know what you want me to do. |
|
Let's leave it as you have it. It's the most efficient way (adding an await will add a delay). Can you please add a test? Maybe in |
cdc1a26 to
704a0e9
Compare
Done |
extmod/uasyncio/stream.py
Outdated
| await self.task | ||
| try: | ||
| await self.task | ||
| except core.CancelledError: |
There was a problem hiding this comment.
Does this now unintentionally suppress cancellations of tasks that wait on the server? Eg:
async def task():
srv = asyncio.start_server(...)
srv.close()
await srv.wait_closed()
async def main():
t = asyncio.create_task(task())
asyncio.sleep(0)
t.cancel() # CancelledError will be suppressed?There was a problem hiding this comment.
Yes, you're right.
If the cancellation is due to Server.close() then the CancelledError should be suppressed. I have pushed a version that makes this work, but it's much more code.
Perhaps we should go with the sleep() version instead (which doesn't have this issue)?
There was a problem hiding this comment.
I think the sleep version is the way to go. I've pushed that and updated the tests with a case that was broken with the initial version of this PR based on your suggestion above.
There was a problem hiding this comment.
Very good.
(There may be some internal state in Task that allows us to determine if the task is scheduled for the first time or not, but I think using that (if it exists) would make it too complex.)
b2442f0 to
534dbd5
Compare
53c6ba7 to
fde0191
Compare
|
Updated. This turned out to be a bit more complicated to deal with cancellation (as usual!) plus a detour to #8929, but it now matches CPython on net_inet/uasyncio_start_server.py and correctly closes the listening socket in all cases. |
fde0191 to
d42fed5
Compare
|
Code size report: |
The following code:
server = await asyncio.start_server(...)
async with server:
... code that raises ...
would lose the original exception because the server's task would not have
had a chance to be scheduled yet, and so awaiting the task in wait_closed
would raise the cancellation instead of the original exception.
Additionally, ensures that explicitly cancelling the parent task delivers
the cancellation correctly (previously was masked by the server loop), now
this only happens if the server was closed, not when the task was
cancelled.
Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
d42fed5 to
977dc9a
Compare
|
This was +80 bytes on stm32... that's a lot... I renamed the The main increase in code size comes from the |
Fix a couple of incorrect type annotations
|
sorry about that, ignore my references here 😦 |
Originally reported here: https://forum.micropython.org/viewtopic.php?f=20&t=12678&p=68845#p68791
The following code:
would lose the original exception because the server's task would not have had a chance to be scheduled yet, and so awaiting the task in wait_closed would raise the cancellation instead of the original exception.
In this case the code that raised was calling
server.serve_forever()which we don't support.