From: Ben Darnell <ben@bendarnell.com>
Date: Sat, 1 Feb 2020 17:49:13 +0000 (-0500)
Subject: iostream: Expand comments around recent subtle changes
X-Git-Tag: v6.0.4^2~8
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=67e47d5e2b84e54eba0294333311ec34cde48072;p=thirdparty%2Ftornado.git

iostream: Expand comments around recent subtle changes
---

diff --git a/tornado/iostream.py b/tornado/iostream.py
index 937eca9b4..6f9b93350 100644
--- a/tornado/iostream.py
+++ b/tornado/iostream.py
@@ -804,8 +804,22 @@ class BaseIOStream(object):
 
     def _start_read(self) -> Future:
         if self._read_future is not None:
-            # raise StreamClosedError instead of assert
-            # in case of starting a second read after the stream is closed
+            # It is an error to start a read while a prior read is unresolved.
+            # However, if the prior read is unresolved because the stream was
+            # closed without satisfying it, it's better to raise
+            # StreamClosedError instead of AssertionError. In particular, this
+            # situation occurs in harmless situations in http1connection.py and
+            # an AssertionError would be logged noisily.
+            #
+            # On the other hand, it is legal to start a new read while the
+            # stream is closed, in case the read can be satisfied from the
+            # read buffer. So we only want to check the closed status of the
+            # stream if we need to decide what kind of error to raise for
+            # "already reading".
+            #
+            # These conditions have proven difficult to test; we have no
+            # unittests that reliably verify this behavior so be careful
+            # when making changes here. See #2651 and #2719.
             self._check_closed()
             assert self._read_future is None, "Already reading"
         self._read_future = Future()