Skip to content

gh-98703: Fix asyncio proactor_events calling _call_connection_lost multiple times#98704

Merged
Fidget-Spinner merged 4 commits intopython:mainfrom
Fidget-Spinner:fix_asyncio_test_drain_release
Oct 27, 2022
Merged

gh-98703: Fix asyncio proactor_events calling _call_connection_lost multiple times#98704
Fidget-Spinner merged 4 commits intopython:mainfrom
Fidget-Spinner:fix_asyncio_test_drain_release

Conversation

@Fidget-Spinner
Copy link
Copy Markdown
Member

@Fidget-Spinner Fidget-Spinner commented Oct 26, 2022

@Fidget-Spinner
Copy link
Copy Markdown
Member Author

@kumaraditya303 I don't know why but this started showing on our tests after #98572. I can't tell if that PR introduced a bug or it just exposed something wrong with our implementation.

IMO, it seems it's something wrong with our implementation.

@kumaraditya303
Copy link
Copy Markdown
Contributor

asyncio always has surprises hence it is my favorite :)

Seriously though this has uncovered another race in our implementation. The code does not guard against concurrent closing of the stream which leads to this failure. The connection_lost callback should be called only once and here it is being called more than once.

Here's a better fix:

diff --git a/Lib/asyncio/proactor_events.py b/Lib/asyncio/proactor_events.py
index 2685a3376c..c6aab408fc 100644
--- a/Lib/asyncio/proactor_events.py
+++ b/Lib/asyncio/proactor_events.py
@@ -152,6 +152,8 @@ def _force_close(self, exc):
         self._loop.call_soon(self._call_connection_lost, exc)
 
     def _call_connection_lost(self, exc):
+        if self._called_connection_lost:
+            return
         try:
             self._protocol.connection_lost(exc)
         finally:

Feel free to update this PR with the patch above, I have verified this.

kumaraditya303