code review 4550112: http: don't fail on accept hitting EMFILE

http: don't fail on accept hitting EMFILE

Fixes issue 1891

[bradfitz, 2011-06-02 21:55:37]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

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

[bradfitz, 2011-06-02 21:57:37]

R=iant too

Typed rsc out of muscle memory, but several people besides me probably know
what's correct here more than I do.

All I know is it is very easy to stop a Go http server right now:
http://code.google.com/p/go/issues/detail?id=1891


On Thu, Jun 2, 2011 at 2:55 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> http: don't fail on accept hitting EMFILE
>
> Please review this at http://codereview.appspot.com/4550112/
>
> Affected files:
>  M src/pkg/http/server.go
>
>
> Index: src/pkg/http/server.go
> ===================================================================
> --- a/src/pkg/http/server.go
> +++ b/src/pkg/http/server.go
> @@ -24,6 +24,7 @@
>        "strconv"
>        "strings"
>        "sync"
> +       "syscall"
>        "time"
>  )
>
> @@ -848,6 +849,15 @@
>        return srv.Serve(l)
>  }
>
> +func errorIsAcceptMaxFiles(err os.Error) bool {
> +       ne, ok := err.(*net.OpError)
> +       if !ok || ne.Op != "accept" {
> +               return false
> +       }
> +       errno, ok := ne.Error.(os.Errno)
> +       return ok && errno == os.Errno(syscall.EMFILE)
> +}
> +
>  // Serve accepts incoming connections on the Listener l, creating a
>  // new service thread for each.  The service threads read requests and
>  // then call srv.Handler to reply to them.
> @@ -860,6 +870,10 @@
>        for {
>                rw, e := l.Accept()
>                if e != nil {
> +                       if errorIsAcceptMaxFiles(e) {
> +                               log.Printf("http: Accept error: %v", e)
> +                               continue
> +                       }
>                        return e
>                }
>                if srv.ReadTimeout != 0 {
>
>
>

[rsc, 2011-06-03 01:46:00]

The correct fix is to edit the definition of Errno.Temporary in package os.
Then check in the accept loop if e implements net.Error and if so
call its Temporary method.

Russ

[bradfitz, 2011-06-03 19:04:06]

Oh, hadn't seem that.  Done.

On Thu, Jun 2, 2011 at 6:45 PM, Russ Cox <rsc@golang.org> wrote:

> The correct fix is to edit the definition of Errno.Temporary in package os.
> Then check in the accept loop if e implements net.Error and if so
> call its Temporary method.
>
> Russ
>

[bradfitz, 2011-06-03 19:04:12]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-03 19:05:55]

http://codereview.appspot.com/4550112/diff/8001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4550112/diff/8001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:851: func errorIsTemporary(err os.Error) bool {
delete

http://codereview.appspot.com/4550112/diff/8001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:872: if errorIsTemporary(e) {
if e, ok := e.(net.Error); e.Temporary() {

http://codereview.appspot.com/4550112/diff/8001/src/pkg/os/error_posix.go
File src/pkg/os/error_posix.go (right):

http://codereview.appspot.com/4550112/diff/8001/src/pkg/os/error_posix.go#new...
src/pkg/os/error_posix.go:16: return e == Errno(syscall.EINTR) || e.Timeout() ||
e == Errno(syscall.EMFILE)
maybe move next to the EINTR check so Timeout stays last?

[rsc, 2011-06-03 19:06:41]

http://codereview.appspot.com/4550112/diff/8001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4550112/diff/8001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:872: if errorIsTemporary(e) {
On 2011/06/03 19:05:55, rsc wrote:
> if e, ok := e.(net.Error); e.Temporary() {
sorry meant ok && e.Temporary()

[bradfitz, 2011-06-03 19:10:41]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-03 19:15:17]

LGTM

[bradfitz, 2011-06-03 19:24:01]

*** Submitted as http://code.google.com/p/go/source/detail?r=95448425041c ***

http: don't fail on accept hitting EMFILE

Fixes issue 1891

R=rsc
CC=golang-dev
http://codereview.appspot.com/4550112