• Brad Fitzpatrick's avatar
    net/http: clean up Transport.RoundTrip error handling · 9d29be46
    Brad Fitzpatrick authored
    If I put a 10 millisecond sleep at testHookWaitResLoop, before the big
    select in (*persistConn).roundTrip, two flakes immediately started
    happening, TestTransportBodyReadError (#19231) and
    TestTransportPersistConnReadLoopEOF.
    
    The problem was that there are many ways for a RoundTrip call to fail
    (errors reading from Request.Body while writing the response, errors
    writing the response, errors reading the response due to server
    closes, errors due to servers sending malformed responses,
    cancelations, timeouts, etc.), and many of those failures then tear
    down the TCP connection, causing more failures, since there are always
    at least three goroutines involved (reading, writing, RoundTripping).
    
    Because the errors were communicated over buffered channels to a giant
    select, the error returned to the caller was a function of which
    random select case was called, which was why a 10ms delay before the
    select brought out so many bugs. (several fixed in my previous CLs the past
    few days).
    
    Instead, track the error explicitly in the transportRequest, guarded
    by a mutex.
    
    In addition, this CL now:
    
    * differentiates between the two ways writing a request can fail: the
      io.Copy reading from the Request.Body or the io.Copy writing to the
      network. A new io.Reader type notes read errors from the
      Request.Body. The read-from-body vs write-to-network errors are now
      prioritized differently.
    
    * unifies the two mapRoundTripErrorFromXXX methods into one
      mapRoundTripError method since their logic is now the same.
    
    * adds a (*Request).WithT(*testing.T) method in export_test.go, usable
      by tests, to call t.Logf at points during RoundTrip. This is disabled
      behind a constant except when debugging.
    
    * documents and deflakes TestClientRedirectContext
    
    I've tested this CL with high -count values, with/without -race,
    with/without delays before the select, etc. So far it seems robust.
    
    Fixes #19231 (TestTransportBodyReadError flake)
    Updates #14203 (source of errors unclear; they're now tracked more)
    Updates #15935 (document Transport errors more; at least understood more now)
    
    Change-Id: I3cccc3607f369724b5344763e35ad2b7ea415738
    Reviewed-on: https://go-review.googlesource.com/37495
    Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
    TryBot-Result: Gobot Gobot <gobot@golang.org>
    Reviewed-by: 's avatarJosh Bleecher Snyder <josharian@gmail.com>
    9d29be46
transport_internal_test.go 3.16 KB