• Brad Fitzpatrick's avatar
    net/http: rework CloseNotifier implementation, clarify expectations in docs · 99fb1919
    Brad Fitzpatrick authored
    CloseNotifier wasn't well specified previously. This CL simplifies its
    implementation, clarifies the public documentation on CloseNotifier,
    clarifies internal documentation on conn, and fixes two CloseNotifier
    bugs in the process.
    
    The main change, though, is tightening the rules and expectations for using
    CloseNotifier:
    
    * the caller must consume the Request.Body first (old rule, unwritten)
    * the received value is the "true" value (old rule, unwritten)
    * no promises for channel sends after Handler returns (old rule, unwritten)
    * a subsequent pipelined request fires the CloseNotifier (new behavior;
      previously it never fired and thus effectively deadlocked as in #13165)
    * advise that it should only be used without HTTP/1.1 pipelining (use HTTP/2
      or non-idempotent browsers). Not that browsers actually use pipelining.
    
    The main implementation change is that each Handler now gets its own
    CloseNotifier channel value, rather than sharing one between the whole
    conn. This means Handlers can't affect subsequent requests. This is
    how HTTP/2's Server works too. The old docs never clarified a behavior
    either way. The other side effect of each request getting its own
    CloseNotifier channel is that one handler can't "poison" the
    underlying conn preventing subsequent requests on the same connection
    from using CloseNotifier (this is #9763).
    
    In the old implementation, once any request on a connection used
    ClosedNotifier, the conn's underlying bufio.Reader source was switched
    from the TCPConn to the read side of the pipe being fed by a
    never-ending copy. Since it was impossible to abort that never-ending
    copy, we could never get back to a fresh state where it was possible
    to return the underlying TCPConn to callers of Hijack. Now, instead of
    a never-ending Copy, the background goroutine doing a Read from the
    TCPConn (or *tls.Conn) only reads a single byte. That single byte
    can be in the request body, a socket timeout error, io.EOF error, or
    the first byte of the second body. In any case, the new *connReader
    type stitches sync and async reads together like an io.MultiReader. To
    clarify the flow of Read data and combat the complexity of too many
    wrapper Reader types, the *connReader absorbs the io.LimitReader
    previously used for bounding request header reads.  The
    liveSwitchReader type is removed. (an unused switchWriter type is also
    removed)
    
    Many fields on *conn are also documented more fully.
    
    Fixes #9763 (CloseNotify + Hijack together)
    Fixes #13165 (deadlock with CloseNotify + pipelined requests)
    
    Change-Id: I40abc0a1992d05b294d627d1838c33cbccb9dd65
    Reviewed-on: https://go-review.googlesource.com/17750Reviewed-by: 's avatarRuss Cox <rsc@golang.org>
    Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
    TryBot-Result: Gobot Gobot <gobot@golang.org>
    99fb1919
serve_test.go 107 KB