Stop requiring an async operation that raises an exception to act as a checkpoint

docs/source/design.rst

@@ -221,6 +221,13 @@ In fact, to make this even simpler, we make it so you don't even have
 to look at the function arguments: each *function* is either a
 cancel+schedule point on *every* call or on *no* calls.
 
+(Pragmatic exception: a Trio primitive is not required to act as a
+cancel+schedule point when it raises an exception, even if it would
+act as one in the case of a successful return. See `issue 474
+<https://github.com/python-trio/trio/issues/474>`__ for more details;
+basically, requiring checkpoints on all exception paths added a lot of
+implementation complexity with negligible user-facing benefit.)
+
 Observation: since blocking is always a cancel+schedule point, rule 2
 implies that any function that *sometimes* blocks is *always* a
 cancel+schedule point.

docs/source/reference-core.rst

@@ -74,8 +74,7 @@ not others, depending on the arguments passed / network speed / phase
 of the moon? How do we figure out where the checkpoints are when
 we're stressed and sleep deprived but still want to get this code
 review right, and would prefer to reserve our mental energy for
-thinking about the actual logic instead of worrying about check
-points?
+thinking about the actual logic instead of worrying about checkpoints?
 
 .. _checkpoint-rule:
 
@@ -85,19 +84,28 @@ them. Here are the rules:
 
 * Regular (synchronous) functions never contain any checkpoints.
 
-* Every async function provided by Trio *always* acts as a check
-  point; if you see ``await <something in trio>``, or ``async for
-  ... in <a trio object>``, or ``async with <trio.something>``, then
-  that's *definitely* a checkpoint.
-
-  (Partial exception: for async context managers, it might be only the
-  entry or only the exit that acts as a checkpoint; this is
-  documented on a case-by-case basis.)
-
-* Third-party async functions can act as checkpoints; if you see
-  ``await <something>`` or one of its friends, then that *might* be a
-  checkpoint. So to be safe, you should prepare for scheduling or
-  cancellation happening there.
+* If you call an async function provided by Trio (``await
+  <something in trio>``), and it doesn't raise an exception,
+  then it *always* acts as a checkpoint. (If it does raise an
+  exception, it might act as a checkpoint or might not.)
+
+  * This includes async iterators: If you write ``async for ... in <a
+    trio object>``, then there will be at least one checkpoint before
+    each iteration of the loop and one checkpoint after the last
+    iteration.
+
+  * Partial exception for async context managers:
+    Both the entry and exit of an ``async with`` block are
+    defined as async functions; but for a
+    particular type of async context manager, it's often the
+    case that only one of them is able to block, which means
+    only that one will act as a checkpoint. This is documented
+    on a case-by-case basis.
+
+* Third-party async functions / iterators / context managers can act
+  as checkpoints; if you see ``await <something>`` or one of its
+  friends, then that *might* be a checkpoint. So to be safe, you
+  should prepare for scheduling or cancellation happening there.
 
 The reason we distinguish between Trio functions and other functions
 is that we can't make any guarantees about third party
@@ -138,8 +146,8 @@ A slightly trickier case is a function like::
 
 Here the function acts as a checkpoint if you call it with
 ``should_sleep`` set to a true value, but not otherwise. This is why
-we emphasize that Trio's own async functions are *unconditional* check
-points: they *always* check for cancellation and check for scheduling,
+we emphasize that Trio's own async functions are *unconditional* checkpoints:
+they *always* check for cancellation and check for scheduling,
 regardless of what arguments they're passed. If you find an async
 function in Trio that doesn't follow this rule, then it's a bug and
 you should `let us know

docs/source/reference-hazmat.rst

@@ -412,10 +412,6 @@ checkpoint semantics. Example::
        except BlockingIOError:
            # need to block and then retry, which we do below
            pass
-       except:
-           # some other error, finish the checkpoint then let it propagate
-           await cancel_shielded_checkpoint()
-           raise
        else:
            # operation succeeded, finish the checkpoint then return
            await cancel_shielded_checkpoint()
@@ -429,13 +425,13 @@ checkpoint semantics. Example::
 
 This logic is a bit convoluted, but accomplishes all of the following:
 
-* Every execution path passes through a checkpoint (assuming that
+* Every successful execution path passes through a checkpoint (assuming that
   ``wait_for_operation_to_be_ready`` is an unconditional checkpoint)
 
 * Our :ref:`cancellation semantics <cancellable-primitives>` say that
   :exc:`~trio.Cancelled` should only be raised if the operation didn't
   happen. Using :func:`cancel_shielded_checkpoint` on the early-exit
-  branches accomplishes this.
+  branch accomplishes this.
 
 * On the path where we do end up blocking, we don't pass through any
   schedule points before that, which avoids some unnecessary work.

newsfragments/474.misc.rst

@@ -0,0 +1,8 @@
+We've slightly relaxed our definition of which Trio operations act as
+:ref:`checkpoints <checkpoint-rule>`. A Trio async function that exits by
+throwing an exception is no longer guaranteed to execute a checkpoint;
+it might or might not. The rules are unchanged for async functions that
+don't exit with an exception, async iterators, and async context managers.
+:func:`trio.testing.assert_checkpoints` has been updated to reflect the
+new behavior: if its ``with`` block exits with an exception, no assertion
+is made.

trio/_core/_io_epoll.py

@@ -103,7 +103,6 @@ async def _epoll_wait(self, fd, attr_name):
             self._registered[fd] = EpollWaiters()
         waiters = self._registered[fd]
         if getattr(waiters, attr_name) is not None:
-            await _core.checkpoint()
             raise _core.BusyResourceError(
                 "another task is already reading / writing this fd"
             )

trio/_core/_io_kqueue.py

@@ -97,7 +97,6 @@ def monitor_kevent(self, ident, filter):
     async def wait_kevent(self, ident, filter, abort_func):
         key = (ident, filter)
         if key in self._registered:
-            await _core.checkpoint()
             raise _core.BusyResourceError(
                 "attempt to register multiple listeners for same "
                 "ident/filter pair"

trio/_core/_io_windows.py

@@ -431,7 +431,6 @@ async def _wait_socket(self, which, sock):
         if not isinstance(sock, int):
             sock = sock.fileno()
         if sock in self._socket_waiters[which]:
-            await _core.checkpoint()
             raise _core.BusyResourceError(
                 "another task is already waiting to {} this socket"
                 .format(which)

trio/_core/_run.py

@@ -898,7 +898,7 @@ class Task:
     _child_nurseries = attr.ib(default=attr.Factory(list))
 
     # these are counts of how many cancel/schedule points this task has
-    # executed, for assert{_no,}_yields
+    # executed, for assert{_no,}_checkpoints
     # XX maybe these should be exposed as part of a statistics() method?
     _cancel_points = attr.ib(default=0)
     _schedule_points = attr.ib(default=0)

trio/_core/tests/test_io.py

@@ -156,9 +156,8 @@ async def test_double_read(socketpair, wait_readable):
     async with _core.open_nursery() as nursery:
         nursery.start_soon(wait_readable, a)
         await wait_all_tasks_blocked()
-        with assert_checkpoints():
-            with pytest.raises(_core.BusyResourceError):
-                await wait_readable(a)
+        with pytest.raises(_core.BusyResourceError):
+            await wait_readable(a)
         nursery.cancel_scope.cancel()
 
 
@@ -171,9 +170,8 @@ async def test_double_write(socketpair, wait_writable):
     async with _core.open_nursery() as nursery:
         nursery.start_soon(wait_writable, a)
         await wait_all_tasks_blocked()
-        with assert_checkpoints():
-            with pytest.raises(_core.BusyResourceError):
-                await wait_writable(a)
+        with pytest.raises(_core.BusyResourceError):
+            await wait_writable(a)
         nursery.cancel_scope.cancel()
 
 

trio/_highlevel_socket.py

@@ -95,17 +95,16 @@ def __init__(self, socket):
 
     async def send_all(self, data):
         if self.socket.did_shutdown_SHUT_WR:
-            await trio.hazmat.checkpoint()
             raise trio.ClosedResourceError("can't send data after sending EOF")
-        with self._send_conflict_detector.sync:
+        with self._send_conflict_detector:
             with _translate_socket_errors_to_stream_errors():
                 with memoryview(data) as data:
                     if not data:
-                        await trio.hazmat.checkpoint()
                         if self.socket.fileno() == -1:
                             raise trio.ClosedResourceError(
                                 "socket was already closed"
                             )
+                        await trio.hazmat.checkpoint()
                         return
                     total_sent = 0
                     while total_sent < len(data):
@@ -114,14 +113,15 @@ async def send_all(self, data):
                         total_sent += sent
 
     async def wait_send_all_might_not_block(self):
-        async with self._send_conflict_detector:
+        with self._send_conflict_detector:
             if self.socket.fileno() == -1:
                 raise trio.ClosedResourceError
             with _translate_socket_errors_to_stream_errors():
                 await self.socket.wait_writable()
 
     async def send_eof(self):
-        async with self._send_conflict_detector:
+        with self._send_conflict_detector:
+            await trio.hazmat.checkpoint()
             # On macOS, calling shutdown a second time raises ENOTCONN, but
             # send_eof needs to be idempotent.
             if self.socket.did_shutdown_SHUT_WR:
@@ -131,7 +131,6 @@ async def send_eof(self):
 
     async def receive_some(self, max_bytes):
         if max_bytes < 1:
-            await trio.hazmat.checkpoint()
             raise ValueError("max_bytes must be >= 1")
         with _translate_socket_errors_to_stream_errors():
             return await self.socket.recv(max_bytes)
@@ -383,7 +382,5 @@ async def aclose(self):
         """Close this listener and its underlying socket.
 
         """
-        try:
-            self.socket.close()
-        finally:
-            await trio.hazmat.checkpoint()
+        self.socket.close()
+        await trio.hazmat.checkpoint()

trio/_signals.py

@@ -107,7 +107,7 @@ async def __anext__(self):
         # In principle it would be possible to support multiple concurrent
         # calls to __anext__, but doing it without race conditions is quite
         # tricky, and there doesn't seem to be any point in trying.
-        with self._conflict_detector.sync:
+        with self._conflict_detector:
             await self._have_pending.wait()
             signum, _ = self._pending.popitem(last=False)
             if not self._pending:

trio/_socket.py

@@ -456,7 +456,6 @@ def close(self):
         self._sock.close()
 
     async def bind(self, address):
-        await trio.hazmat.checkpoint()
         address = await self._resolve_local_address(address)
         if (
             hasattr(_stdlib_socket, "AF_UNIX")
@@ -493,11 +492,9 @@ async def _resolve_address(self, address, flags):
         # Do some pre-checking (or exit early for non-IP sockets)
         if self._sock.family == _stdlib_socket.AF_INET:
             if not isinstance(address, tuple) or not len(address) == 2:
-                await trio.hazmat.checkpoint()
                 raise ValueError("address should be a (host, port) tuple")
         elif self._sock.family == _stdlib_socket.AF_INET6:
             if not isinstance(address, tuple) or not 2 <= len(address) <= 4:
-                await trio.hazmat.checkpoint()
                 raise ValueError(
                     "address should be a (host, port, [flowinfo, [scopeid]]) "
                     "tuple"