code review 70010050: net/http: fix test failure on some Windows machines

net/http: fix test failure on some Windows machines

The network connection dies differently from the server's
perspective on (some) Windows when the client goes away. Match
on the common prefix (common to Unix and Windows) instead of
the network error part.

Fixes Issue 7456

[bradfitz, 2014-03-04 17:16:41]

Hello golang-codereviews@googlegroups.com (cc: alex.brainman@gmail.com,
iant@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[josharian, 2014-03-04 19:49:38]

LGTM

[bradfitz, 2014-03-04 19:55:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=50d6301e44f1 ***

net/http: fix test failure on some Windows machines

The network connection dies differently from the server's
perspective on (some) Windows when the client goes away. Match
on the common prefix (common to Unix and Windows) instead of
the network error part.

Fixes Issue 7456

LGTM=josharian
R=golang-codereviews, josharian
CC=alex.brainman, golang-codereviews, iant
https://codereview.appspot.com/70010050

[brainman, 2014-03-05 01:36:56]

I have concerns about you ignoring the nature of the error:

C:\go\root\src>go test -run=IncorrectTLSServerName -v net/http
=== RUN TestClientWithIncorrectTLSServerName
--- FAIL: TestClientWithIncorrectTLSServerName (0.55 seconds)
        client_test.go:675: expected an error log message containing 'bad
certificate'; got "http: TLS handshake error from 127.0.0.1:1508: WSARecv tcp
127.0.0.1:1507: An existing connection was forcibly closed by the remote
host.\n"
FAIL

The connection is terminated in an unexpected (for you) way here. On some pcs
both client and server get all appropriate data before connection is terminated,
while here someone is not received their data. This could be a problem with
net/http/httptest package or, even worse, net or runtime. Do you care for me to
investigate this case further? Or you think it is not worth pursuing?

Alex

[bradfitz, 2014-03-05 01:44:29]

No, the TestClientWithIncorrectTLSServerName is a test of the client (it's
in client_test.go).  All we really care about is whether the client saw the
correct error.

This buggy test isn't even very necessary: I added it in only the other day
when Server.ErrorLog became available (in
https://code.google.com/p/go/source/detail?r=26162125c6bd#), just to shut
up some useless noise in the tests.  Previously the server was logging to
os.Stderr when the client forcefully disconnected after it (correctly)
rejected the server's bad TLS cert name.  I figured rather than just log
the error to ioutil.Discard, I would even look at it.  I was on a Unix
machine at the time, so I just matched on some substring of it without
thinking. On that machine, it just so happens that the server saw a TCP/IP
error that looked like that.  The server's perspective can vary based on
what/when the client wrote/flushed, RST vs ACKs, etc.

tl;dr: we don't care about the server's output.



On Tue, Mar 4, 2014 at 5:36 PM, <alex.brainman@gmail.com> wrote:

> I have concerns about you ignoring the nature of the error:
>
> C:\go\root\src>go test -run=IncorrectTLSServerName -v net/http
>
> === RUN TestClientWithIncorrectTLSServerName
> --- FAIL: TestClientWithIncorrectTLSServerName (0.55 seconds)
>         client_test.go:675: expected an error log message containing
> 'bad certificate'; got "http: TLS handshake error from 127.0.0.1:1508:
> WSARecv tcp 127.0.0.1:1507: An existing connection was forcibly closed
> by the remote host.\n"
> FAIL
>
> The connection is terminated in an unexpected (for you) way here. On
> some pcs both client and server get all appropriate data before
> connection is terminated, while here someone is not received their data.
> This could be a problem with net/http/httptest package or, even worse,
> net or runtime. Do you care for me to investigate this case further? Or
> you think it is not worth pursuing?
>
> Alex
>
> https://codereview.appspot.com/70010050/
>