Server.Serve exits even on temporary errors

[drdaeman]

Hey, thank you for this library! I was experimenting with custom net.Listeners and it seems that Serve exits on any error from l.Accept unconditionally:

go-smtp/server.go

Lines 113 to 122 in f9aa4f4

	c, err := l.Accept()
	if err != nil {
	select {
	case <-s.done:
	// we called Close()
	return nil
	default:
	return err
	}
	}

Unfortunately, this essentially causes server to shut down upon non-fatal errors. This potentially allows a DoS attack (shutting server down by maliciously closing connections as they're about to get accepted), although this is only my suspicion that I haven't properly validated. IIRC, TLS listener may return net.ErrClosed if something bad happens during the handshake, and even plain TCP connections may accidentally fail with ECONNRESET or ECONNABORTED. Either way, I believe a slightly more sophisticated loop would be an improvement.

Here's an adapted version based on how net/http does it:

var tempDelay time.Duration // how long to sleep on accept failure

for {
	c, err := l.Accept()
	if err != nil {
		select {
		case <-s.done:
			// we called Close()
			return nil
		default:
		}
		if ne, ok := err.(net.Error); ok && ne.Temporary() {
			if tempDelay == 0 {
				tempDelay = 5 * time.Millisecond
			} else {
				tempDelay *= 2
			}
			if max := 1 * time.Second; tempDelay > max {
				tempDelay = max
			}
			s.ErrorLog.Printf("accept error: %v; retrying in %v", err, tempDelay)
			time.Sleep(tempDelay)
			continue
		}
		return err
	}
	go s.handleConn(newConn(c, s))
}

(Possibly somewhat contradictory, those errors are marked as "temporary" - see golang/go#6163 for more details).

[emersion]

Indeed, we shouldn't exit on temporary Accept errors. Patches welcome!