code review 5312048: http: add ECONNRESET to remote side closed detection on...

http: add ECONNRESET to remote side closed detection on windows

[brainman, 2011-10-21 05:43:41]

Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org,
hectorchu@gmail.com),

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

[brainman, 2011-10-21 05:44:19]

I think this bit of code belongs somewhere in the net package.

Alex

[bradfitz, 2011-10-21 06:42:12]

LGTM

.. with one fear:

http://codereview.appspot.com/5312048/diff/5001/src/pkg/http/transport_window...
File src/pkg/http/transport_windows.go (right):

http://codereview.appspot.com/5312048/diff/5001/src/pkg/http/transport_window...
src/pkg/http/transport_windows.go:17: switch op.Error.(os.Errno) {
is this type assertion safe?

I dislike the single lvalue panicing version of type assertions.  Too
error-prone.

I'd rather this be:

errno, _ := op.Error(os.Errno)
switch errno {
...
}

[hector, 2011-10-21 08:54:31]

http://codereview.appspot.com/5312048/diff/5001/src/pkg/http/transport_window...
File src/pkg/http/transport_windows.go (right):

http://codereview.appspot.com/5312048/diff/5001/src/pkg/http/transport_window...
src/pkg/http/transport_windows.go:18: case syscall.WSAESHUTDOWN,
syscall.WSAECONNRESET:
I thought the error was ERROR_NETNAME_DELETED?
I think it was Linux that was ECONNRESET, which should be added to the common
error handling instead.

[rsc, 2011-10-21 14:47:12]

http://codereview.appspot.com/5312048/diff/5001/src/pkg/http/transport_window...
File src/pkg/http/transport_windows.go (right):

http://codereview.appspot.com/5312048/diff/5001/src/pkg/http/transport_window...
src/pkg/http/transport_windows.go:5: package http
Can this file go way?  It was fine as a workaround, but package net
should not be letting OS-specific details through like this.  Whatever
the condition is that causes this error should produce the same behavior
by package net regardless of underlying operating system.

[brainman, 2011-10-24 05:09:02]

On 2011/10/21 14:47:12, rsc wrote:
> ... but package net
> should not be letting OS-specific details through like this. ...

Good point.

> ... Whatever
> the condition is that causes this error should produce the same behavior
> by package net regardless of underlying operating system.

The function is called remoteSideClosed. So that is the condition we are after.
But what does it practically mean? How does this condition gets triggered? And
can we
recognize this condition just by looking at error message. I doubt.

Brad, I don't know your code well enough, but why can't you just exit readLoop
if Peek
returns any error. Why do you differentiate between different errors?

Alex

[brainman, 2011-12-07 04:32:57]

*** Abandoned ***