Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-79033: Fix asyncio.Server.wait_closed() #98582

Merged
merged 4 commits into from
Nov 24, 2022

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Oct 24, 2022

wait_closed() was a no-op when used as recommended (after close()).

I had to debug one test (test__sock_sendfile_native_failure) -- the cleanup sequence for the test fixture was botched.

Hopefully that's not a portend of problems in user code -- this has never worked so people may well be doing this wrong. :-(

It was a no-op when used as recommended (after close()).

I had to debug one test (test__sock_sendfile_native_failure) --
the cleanup sequence for the test fixture was botched.

Hopefully that's not a portend of problems in user code --
this has never worked so people may well be doing this wrong. :-(
@gvanrossum
Copy link
Member Author

Needs test and news entry

@gvanrossum
Copy link
Member Author

@kumaraditya303 Can you suggest a test for this?

@kumaraditya303
Copy link
Contributor

Something like should work:

diff --git a/Lib/test/test_asyncio/test_server.py b/Lib/test/test_asyncio/test_server.py
index 860d62d52e..ceea4059ae 100644
--- a/Lib/test/test_asyncio/test_server.py
+++ b/Lib/test/test_asyncio/test_server.py
@@ -120,6 +120,26 @@ async def main(srv):
                 self.loop.run_until_complete(srv.serve_forever())
 
 
+class TestServer2(unittest.IsolatedAsyncioTestCase):
+
+    async def test_wait_closed(self):
+        async def serve(*args):
+            pass
+
+        srv = await asyncio.start_server(serve, socket_helper.HOSTv4, 0)
+
+        # active count = 0
+        wait = asyncio.create_task(srv.wait_closed())
+        await asyncio.sleep(0)
+        self.assertTrue(wait.done())
+
+        # active count != 0
+        srv._attach()
+        wait = asyncio.create_task(srv.wait_closed())
+        await asyncio.sleep(0)
+        self.assertFalse(wait.done())
+
+
 @unittest.skipUnless(hasattr(asyncio, 'ProactorEventLoop'), 'Windows only')
 class ProactorStartServerTests(BaseStartServer, unittest.TestCase):
 

Creating a test with an actual client seems rather flaky as it requires a heavy load on server so will be unreliable.

@gvanrossum
Copy link
Member Author

Thanks! _attach exists for the transport to indicate that it's active. We should probably also call _detach (after closing the server) and then check that the wait_closed task finishes.

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@gvanrossum gvanrossum merged commit 5d09d11 into python:main Nov 24, 2022
@gvanrossum gvanrossum deleted the fix-server-wait-closed branch November 24, 2022 15:33
@gvanrossum
Copy link
Member Author

Let’s not backport, I fear it will surprise some users since it never worked.

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

Successfully merging this pull request may close these issues.

3 participants