code review 97350043: net/http: treat a few more Accept errors as temporary o...

net/http: treat a few more Accept errors as temporary on Linux

Update Issue 6163

[bradfitz, 2014-05-12 16:53:44]

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

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

[albert.strasheim, 2014-05-12 17:08:02]

FWIW, I think this is also an issue on other platforms. Not sure if you want to
address them here.

E.g. FreeBSD has:

http://www.freebsd.org/cgi/man.cgi?query=accept&sektion=2

ECONNABORTED: A connection arrived, but it was closed while waiting on the
listen queue.

Maybe Darwin has the same.

[bradfitz, 2014-05-12 17:13:17]

I structured the code so additions are easy in separate CLs from people who
have time/machines to research.



On Mon, May 12, 2014 at 10:08 AM, <fullung@gmail.com> wrote:

> FWIW, I think this is also an issue on other platforms. Not sure if you
> want to address them here.
>
> E.g. FreeBSD has:
>
> http://www.freebsd.org/cgi/man.cgi?query=accept&sektion=2
>
> ECONNABORTED: A connection arrived, but it was closed while waiting on
> the listen queue.
>
> Maybe Darwin has the same.
>
> https://codereview.appspot.com/97350043/
>

[albert.strasheim, 2014-05-12 17:16:18]

On 2014/05/12 17:13:17, bradfitz wrote:
> I structured the code so additions are easy in separate CLs from people who
> have time/machines to research.

Maybe just Update issue 6163 then?

[bradfitz, 2014-05-12 17:18:48]

Sure, done.



On Mon, May 12, 2014 at 10:16 AM, <fullung@gmail.com> wrote:

> On 2014/05/12 17:13:17, bradfitz wrote:
>
>> I structured the code so additions are easy in separate CLs from
>>
> people who
>
>> have time/machines to research.
>>
>
> Maybe just Update issue 6163 then?
>
> https://codereview.appspot.com/97350043/
>

[mikio, 2014-05-12 17:55:41]

https://codereview.appspot.com/97350043/diff/60001/src/pkg/net/http/http_linu...
File src/pkg/net/http/http_linux.go (right):

https://codereview.appspot.com/97350043/diff/60001/src/pkg/net/http/http_linu...
src/pkg/net/http/http_linux.go:14: if se, ok := err.(syscall.Errno); ok {
fwiw, nested error patterns for IO op in current net package:
a) syscall.Errno, // syscall.EINVAL to indicate broken Conn/netFD
b) &OpError{Err: syscall.Errno or other},
c) &OpError{Err: &os.SyscallError{Err: syscall.Errno or other}.

[rsc, 2014-05-13 03:45:58]

not lgtm

The reason net.Error exists is to avoid code like this.
net/http has no *_linux.go files yet, and I'd really like to keep it that way.

If the Linux implementation of syscall.Errno.Temporary needs to be fixed, let's
fix it in syscall. Patching net/http doesn't help any other network services.

[bradfitz, 2014-05-13 04:08:49]

That's fine with me. I was just nervous touching syscall and changing more
behavior this late.

I only cared about this bug because it was tagged Go-1.3. The workaround is
obvious: provide your own net.Listener wrapper that hides whatever the user
considers a temporary failure.

Happy punting this to Go 1.4, unless you feel like we should change
Errno.Temporary still.




On Mon, May 12, 2014 at 8:45 PM, <rsc@golang.org> wrote:

> not lgtm
>
> The reason net.Error exists is to avoid code like this.
> net/http has no *_linux.go files yet, and I'd really like to keep it
> that way.
>
> If the Linux implementation of syscall.Errno.Temporary needs to be
> fixed, let's fix it in syscall. Patching net/http doesn't help any other
> network services.
>
>
> https://codereview.appspot.com/97350043/
>

[rsc, 2014-05-13 04:34:04]

Let's punt to 1.4 but plan to change Errno.Temporary.

[dvyukov, 2014-07-26 08:22:57]

ping, let's fix it

[dvyukov, 2014-07-26 08:27:20]

On 2014/05/13 03:45:58, rsc wrote:
> not lgtm
> 
> The reason net.Error exists is to avoid code like this.
> net/http has no *_linux.go files yet, and I'd really like to keep it that way.
> 
> If the Linux implementation of syscall.Errno.Temporary needs to be fixed,
let's
> fix it in syscall. Patching net/http doesn't help any other network services.


I do not agree.

There are universally temporary errors, like EAGAIN. And there are errors that
are temporal for a specific syscall. This is the latter case.
If you get ECONNRESET from read syscall, you can retry it until you are blue in
the face; it's not going to go away.

[ioe, 2014-07-26 15:13:16]

On Saturday, July 26, 2014 10:27:21 AM UTC+2, Dmitry Vyukov wrote:
> On 2014/05/13 03:45:58, rsc wrote:
> > The reason net.Error exists is to avoid code like this.
> 
> > net/http has no *_linux.go files yet, and I'd really like to keep it that
way.
> > If the Linux implementation of syscall.Errno.Temporary needs to be fixed,
let's
> > fix it in syscall. Patching net/http doesn't help any other network
services.
> There are universally temporary errors, like EAGAIN. And there are
> errors that are temporal for a specific syscall. This is the latter
> case.
> 
> If you get ECONNRESET from read syscall, you can retry it until you are
> blue in the face; it's not going to go away.

Please note that those errors can be distinguished, as the syscall/operation is
always included in the error. So e.g. OpError.Temporary() could take this into
account.

[rsc, 2014-08-05 17:57:43]

Still NOT LGTM.

Why does this apply only to HTTP servers and not to other kinds of network
servers? This is still the wrong place for this fix.