Issue #25593: Change semantics of EventLoop.stop().

6 files changed, 87 insertions, 28 deletions

diff --git a/Doc/library/asyncio-eventloop.rst b/Doc/library/asyncio-eventloop.rst
index e867c80..96468ae 100644
--- a/Doc/library/asyncio-eventloop.rst
+++ b/Doc/library/asyncio-eventloop.rst
@@ -29,7 +29,16 @@ Run an event loop
 
 .. method:: BaseEventLoop.run_forever()
 
-   Run until :meth:`stop` is called.
+   Run until :meth:`stop` is called.  If :meth:`stop` is called before
+   :meth:`run_forever()` is called, this polls the I/O selector once
+   with a timeout of zero, runs all callbacks scheduled in response to
+   I/O events (and those that were already scheduled), and then exits.
+   If :meth:`stop` is called while :meth:`run_forever` is running,
+   this will run the current batch of callbacks and then exit.  Note
+   that callbacks scheduled by callbacks will not run in that case;
+   they will run the next time :meth:`run_forever` is called.
+
+   .. versionchanged:: 3.4.4
 
 .. method:: BaseEventLoop.run_until_complete(future)
 
@@ -48,10 +57,10 @@ Run an event loop
 
    Stop running the event loop.
 
-   Every callback scheduled before :meth:`stop` is called will run.
-   Callbacks scheduled after :meth:`stop` is called will not run.
-   However, those callbacks will run if :meth:`run_forever` is called
-   again later.
+   This causes :meth:`run_forever` to exit at the next suitable
+   opportunity (see there for more details).
+
+   .. versionchanged:: 3.4.4
 
 .. method:: BaseEventLoop.is_closed()
 
@@ -61,7 +70,8 @@ Run an event loop
 
 .. method:: BaseEventLoop.close()
 
-   Close the event loop. The loop must not be running.
+   Close the event loop. The loop must not be running.  Pending
+   callbacks will be lost.
 
    This clears the queues and shuts down the executor, but does not wait for
    the executor to finish.
diff --git a/Doc/library/asyncio-protocol.rst b/Doc/library/asyncio-protocol.rst
index 9350ffe..78faeae 100644
--- a/Doc/library/asyncio-protocol.rst
+++ b/Doc/library/asyncio-protocol.rst
@@ -494,7 +494,7 @@ data and wait until the connection is closed::
 
         def connection_lost(self, exc):
             print('The server closed the connection')
-            print('Stop the event lop')
+            print('Stop the event loop')
             self.loop.stop()
 
     loop = asyncio.get_event_loop()
diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py
index af9c881..c5ffad4 100644
--- a/Lib/asyncio/base_events.py
+++ b/Lib/asyncio/base_events.py
@@ -70,10 +70,6 @@ def _format_pipe(fd):
         return repr(fd)
 
 
-class _StopError(BaseException):
-    """Raised to stop the event loop."""
-
-
 def _check_resolved_address(sock, address):
     # Ensure that the address is already resolved to avoid the trap of hanging
     # the entire event loop when the address requires doing a DNS lookup.
@@ -118,9 +114,6 @@ def _check_resolved_address(sock, address):
                              "got host %r: %s"
                              % (host, err))
 
-def _raise_stop_error(*args):
-    raise _StopError
-
 
 def _run_until_complete_cb(fut):
     exc = fut._exception
@@ -129,7 +122,7 @@ def _run_until_complete_cb(fut):
         # Issue #22429: run_forever() already finished, no need to
         # stop it.
         return
-    _raise_stop_error()
+    fut._loop.stop()
 
 
 class Server(events.AbstractServer):
@@ -184,6 +177,7 @@ class BaseEventLoop(events.AbstractEventLoop):
     def __init__(self):
         self._timer_cancelled_count = 0
         self._closed = False
+        self._stopping = False
         self._ready = collections.deque()
         self._scheduled = []
         self._default_executor = None
@@ -298,11 +292,11 @@ class BaseEventLoop(events.AbstractEventLoop):
         self._thread_id = threading.get_ident()
         try:
             while True:
-                try:
-                    self._run_once()
-                except _StopError:
+                self._run_once()
+                if self._stopping:
                     break
         finally:
+            self._stopping = False
             self._thread_id = None
             self._set_coroutine_wrapper(False)
 
@@ -345,11 +339,10 @@ class BaseEventLoop(events.AbstractEventLoop):
     def stop(self):
         """Stop running the event loop.
 
-        Every callback scheduled before stop() is called will run. Callbacks
-        scheduled after stop() is called will not run. However, those callbacks
-        will run if run_forever is called again later.
+        Every callback already scheduled will still run.  This simply informs
+        run_forever to stop looping after a complete iteration.
         """
-        self.call_soon(_raise_stop_error)
+        self._stopping = True
 
     def close(self):
         """Close the event loop.
@@ -1194,7 +1187,7 @@ class BaseEventLoop(events.AbstractEventLoop):
                 handle._scheduled = False
 
         timeout = None
-        if self._ready:
+        if self._ready or self._stopping:
             timeout = 0
         elif self._scheduled:
             # Compute the desired timeout.
diff --git a/Lib/asyncio/test_utils.py b/Lib/asyncio/test_utils.py
index 8cee95b..e06ac06 100644
--- a/Lib/asyncio/test_utils.py
+++ b/Lib/asyncio/test_utils.py
@@ -71,12 +71,13 @@ def run_until(loop, pred, timeout=30):
 
 
 def run_once(loop):
-    """loop.stop() schedules _raise_stop_error()
-    and run_forever() runs until _raise_stop_error() callback.
-    this wont work if test waits for some IO events, because
-    _raise_stop_error() runs before any of io events callbacks.
+    """Legacy API to run once through the event loop.
+
+    This is the recommended pattern for test code.  It will poll the
+    selector once and run all callbacks scheduled in response to I/O
+    events.
     """
-    loop.stop()
+    loop.call_soon(loop.stop)
     loop.run_forever()
 
 
diff --git a/Lib/test/test_asyncio/test_base_events.py b/Lib/test/test_asyncio/test_base_events.py
index 54b771e..072fe2f 100644
--- a/Lib/test/test_asyncio/test_base_events.py
+++ b/Lib/test/test_asyncio/test_base_events.py
@@ -757,6 +757,59 @@ class BaseEventLoopTests(test_utils.TestCase):
             pass
         self.assertTrue(func.called)
 
+    def test_single_selecter_event_callback_after_stopping(self):
+        # Python issue #25593: A stopped event loop may cause event callbacks
+        # to run more than once.
+        event_sentinel = object()
+        callcount = 0
+        doer = None
+
+        def proc_events(event_list):
+            nonlocal doer
+            if event_sentinel in event_list:
+                doer = self.loop.call_soon(do_event)
+
+        def do_event():
+            nonlocal callcount
+            callcount += 1
+            self.loop.call_soon(clear_selector)
+
+        def clear_selector():
+            doer.cancel()
+            self.loop._selector.select.return_value = ()
+
+        self.loop._process_events = proc_events
+        self.loop._selector.select.return_value = (event_sentinel,)
+
+        for i in range(1, 3):
+            with self.subTest('Loop %d/2' % i):
+                self.loop.call_soon(self.loop.stop)
+                self.loop.run_forever()
+                self.assertEqual(callcount, 1)
+
+    def test_run_once(self):
+        # Simple test for test_utils.run_once().  It may seem strange
+        # to have a test for this (the function isn't even used!) but
+        # it's a de-factor standard API for library tests.  This tests
+        # the idiom: loop.call_soon(loop.stop); loop.run_forever().
+        count = 0
+
+        def callback():
+            nonlocal count
+            count += 1
+
+        self.loop._process_events = mock.Mock()
+        self.loop.call_soon(callback)
+        test_utils.run_once(self.loop)
+        self.assertEqual(count, 1)
+
+    def test_run_forever_pre_stopped(self):
+        # Test that the old idiom for pre-stopping the loop works.
+        self.loop._process_events = mock.Mock()
+        self.loop.stop()
+        self.loop.run_forever()
+        self.loop._selector.select.assert_called_once_with(0)
+
 
 class MyProto(asyncio.Protocol):
     done = None
diff --git a/Misc/NEWS b/Misc/NEWS
index 2d0755f..e98e4a6 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -106,6 +106,8 @@ Core and Builtins
 Library
 -------
 
+- Issue #25593: Change semantics of EventLoop.stop() in asyncio.
+
 - Issue #6973: When we know a subprocess.Popen process has died, do
   not allow the send_signal(), terminate(), or kill() methods to do
   anything as they could potentially signal a different process.