mirror of
https://github.com/caddyserver/caddy.git
synced 2024-12-16 21:56:40 -05:00
core: Synchronize calls to SetDeadline within fakeCloseListener
First evidenced in #2658, listener deadlines would sometimes be set after clearing them, resulting in endless i/o timeout errors, which leave all requests hanging. This bug is fixed by synchronizing the calls to SetDeadline: when Close() is called, the deadline is first set to a time in the past, and the lock is released only after the deadline is set, so when the other servers break out of their Accept() calls, they will clear the deadline *after* it was set. Before, the clearing could sometimes come before the set, which meant that it was left in a timeout state indefinitely. This may not yet be a perfect solution -- ideally, the setting and clearing of the deadline would happen exactly once per underlying listener, not once per fakeCloseListener, but in rigorous testing with these changes (comprising tens of thousands of config reloads), I was able to verify that no race condition is manifest.
This commit is contained in:
parent
35f70c98fa
commit
27e288ab19
1 changed files with 73 additions and 34 deletions
107
listeners.go
107
listeners.go
|
@ -25,8 +25,6 @@ import (
|
||||||
"time"
|
"time"
|
||||||
)
|
)
|
||||||
|
|
||||||
// TODO: Can we use the new UsagePool type?
|
|
||||||
|
|
||||||
// Listen returns a listener suitable for use in a Caddy module.
|
// Listen returns a listener suitable for use in a Caddy module.
|
||||||
// Always be sure to close listeners when you are done with them.
|
// Always be sure to close listeners when you are done with them.
|
||||||
func Listen(network, addr string) (net.Listener, error) {
|
func Listen(network, addr string) (net.Listener, error) {
|
||||||
|
@ -36,9 +34,15 @@ func Listen(network, addr string) (net.Listener, error) {
|
||||||
defer listenersMu.Unlock()
|
defer listenersMu.Unlock()
|
||||||
|
|
||||||
// if listener already exists, increment usage counter, then return listener
|
// if listener already exists, increment usage counter, then return listener
|
||||||
if lnUsage, ok := listeners[lnKey]; ok {
|
if lnGlobal, ok := listeners[lnKey]; ok {
|
||||||
atomic.AddInt32(&lnUsage.usage, 1)
|
atomic.AddInt32(&lnGlobal.usage, 1)
|
||||||
return &fakeCloseListener{usage: &lnUsage.usage, key: lnKey, Listener: lnUsage.ln}, nil
|
return &fakeCloseListener{
|
||||||
|
usage: &lnGlobal.usage,
|
||||||
|
deadline: &lnGlobal.deadline,
|
||||||
|
deadlineMu: &lnGlobal.deadlineMu,
|
||||||
|
key: lnKey,
|
||||||
|
Listener: lnGlobal.ln,
|
||||||
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// or, create new one and save it
|
// or, create new one and save it
|
||||||
|
@ -48,10 +52,19 @@ func Listen(network, addr string) (net.Listener, error) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// make sure to start its usage counter at 1
|
// make sure to start its usage counter at 1
|
||||||
lnUsage := &listenerUsage{usage: 1, ln: ln}
|
lnGlobal := &globalListener{
|
||||||
listeners[lnKey] = lnUsage
|
usage: 1,
|
||||||
|
ln: ln,
|
||||||
|
}
|
||||||
|
listeners[lnKey] = lnGlobal
|
||||||
|
|
||||||
return &fakeCloseListener{usage: &lnUsage.usage, key: lnKey, Listener: ln}, nil
|
return &fakeCloseListener{
|
||||||
|
usage: &lnGlobal.usage,
|
||||||
|
deadline: &lnGlobal.deadline,
|
||||||
|
deadlineMu: &lnGlobal.deadlineMu,
|
||||||
|
key: lnKey,
|
||||||
|
Listener: ln,
|
||||||
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// ListenPacket returns a net.PacketConn suitable for use in a Caddy module.
|
// ListenPacket returns a net.PacketConn suitable for use in a Caddy module.
|
||||||
|
@ -63,10 +76,10 @@ func ListenPacket(network, addr string) (net.PacketConn, error) {
|
||||||
defer listenersMu.Unlock()
|
defer listenersMu.Unlock()
|
||||||
|
|
||||||
// if listener already exists, increment usage counter, then return listener
|
// if listener already exists, increment usage counter, then return listener
|
||||||
if lnUsage, ok := listeners[lnKey]; ok {
|
if lnGlobal, ok := listeners[lnKey]; ok {
|
||||||
atomic.AddInt32(&lnUsage.usage, 1)
|
atomic.AddInt32(&lnGlobal.usage, 1)
|
||||||
log.Printf("[DEBUG] %s: Usage counter should not go above 2 or maybe 3, is now: %d", lnKey, atomic.LoadInt32(&lnUsage.usage)) // TODO: remove
|
log.Printf("[DEBUG] %s: Usage counter should not go above 2 or maybe 3, is now: %d", lnKey, atomic.LoadInt32(&lnGlobal.usage)) // TODO: remove
|
||||||
return &fakeClosePacketConn{usage: &lnUsage.usage, key: lnKey, PacketConn: lnUsage.pc}, nil
|
return &fakeClosePacketConn{usage: &lnGlobal.usage, key: lnKey, PacketConn: lnGlobal.pc}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// or, create new one and save it
|
// or, create new one and save it
|
||||||
|
@ -76,10 +89,10 @@ func ListenPacket(network, addr string) (net.PacketConn, error) {
|
||||||
}
|
}
|
||||||
|
|
||||||
// make sure to start its usage counter at 1
|
// make sure to start its usage counter at 1
|
||||||
lnUsage := &listenerUsage{usage: 1, pc: pc}
|
lnGlobal := &globalListener{usage: 1, pc: pc}
|
||||||
listeners[lnKey] = lnUsage
|
listeners[lnKey] = lnGlobal
|
||||||
|
|
||||||
return &fakeClosePacketConn{usage: &lnUsage.usage, key: lnKey, PacketConn: pc}, nil
|
return &fakeClosePacketConn{usage: &lnGlobal.usage, key: lnKey, PacketConn: pc}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// fakeCloseListener's Close() method is a no-op. This allows
|
// fakeCloseListener's Close() method is a no-op. This allows
|
||||||
|
@ -87,11 +100,17 @@ func ListenPacket(network, addr string) (net.PacketConn, error) {
|
||||||
// up the socket; thus, servers become hot-swappable while the
|
// up the socket; thus, servers become hot-swappable while the
|
||||||
// listener remains running. Listeners should be re-wrapped in
|
// listener remains running. Listeners should be re-wrapped in
|
||||||
// a new fakeCloseListener each time the listener is reused.
|
// a new fakeCloseListener each time the listener is reused.
|
||||||
|
// Other than the 'closed' field (which pertains to this value
|
||||||
|
// only), the other fields in this struct should be pointers to
|
||||||
|
// the associated globalListener's struct fields (except 'key'
|
||||||
|
// which is there for read-only purposes, so it can be a copy).
|
||||||
type fakeCloseListener struct {
|
type fakeCloseListener struct {
|
||||||
closed int32 // accessed atomically - TODO: this needs to be shared across the whole app instance, not to cross instance boundaries... hmmm... see #2658 (still relevant?)
|
closed int32 // accessed atomically; belongs to this struct only
|
||||||
usage *int32 // accessed atomically
|
usage *int32 // accessed atomically; global
|
||||||
key string
|
deadline *bool // protected by deadlineMu; global
|
||||||
net.Listener
|
deadlineMu *sync.Mutex // global
|
||||||
|
key string // global, but read-only, so can be copy
|
||||||
|
net.Listener // global
|
||||||
}
|
}
|
||||||
|
|
||||||
// Accept accepts connections until Close() is called.
|
// Accept accepts connections until Close() is called.
|
||||||
|
@ -107,15 +126,21 @@ func (fcl *fakeCloseListener) Accept() (net.Conn, error) {
|
||||||
return conn, nil
|
return conn, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
if atomic.LoadInt32(&fcl.closed) == 1 {
|
// accept returned with error
|
||||||
// clear the deadline
|
// TODO: This may be better as a condition variable so the deadline is cleared only once?
|
||||||
|
fcl.deadlineMu.Lock()
|
||||||
|
if *fcl.deadline {
|
||||||
switch ln := fcl.Listener.(type) {
|
switch ln := fcl.Listener.(type) {
|
||||||
case *net.TCPListener:
|
case *net.TCPListener:
|
||||||
ln.SetDeadline(time.Time{})
|
ln.SetDeadline(time.Time{})
|
||||||
case *net.UnixListener:
|
case *net.UnixListener:
|
||||||
ln.SetDeadline(time.Time{})
|
ln.SetDeadline(time.Time{})
|
||||||
}
|
}
|
||||||
|
*fcl.deadline = false
|
||||||
|
}
|
||||||
|
fcl.deadlineMu.Unlock()
|
||||||
|
|
||||||
|
if atomic.LoadInt32(&fcl.closed) == 1 {
|
||||||
// if we cancelled the Accept() by setting a deadline
|
// if we cancelled the Accept() by setting a deadline
|
||||||
// on the listener, we need to make sure any callers of
|
// on the listener, we need to make sure any callers of
|
||||||
// Accept() think the listener was actually closed;
|
// Accept() think the listener was actually closed;
|
||||||
|
@ -141,12 +166,17 @@ func (fcl *fakeCloseListener) Close() error {
|
||||||
// a deadline in the past, which forces it to
|
// a deadline in the past, which forces it to
|
||||||
// time out; note that this only works for
|
// time out; note that this only works for
|
||||||
// certain types of listeners...
|
// certain types of listeners...
|
||||||
switch ln := fcl.Listener.(type) {
|
fcl.deadlineMu.Lock()
|
||||||
case *net.TCPListener:
|
if !*fcl.deadline {
|
||||||
ln.SetDeadline(time.Now().Add(-1 * time.Minute))
|
switch ln := fcl.Listener.(type) {
|
||||||
case *net.UnixListener:
|
case *net.TCPListener:
|
||||||
ln.SetDeadline(time.Now().Add(-1 * time.Minute))
|
ln.SetDeadline(time.Now().Add(-1 * time.Minute))
|
||||||
|
case *net.UnixListener:
|
||||||
|
ln.SetDeadline(time.Now().Add(-1 * time.Minute))
|
||||||
|
}
|
||||||
|
*fcl.deadline = true
|
||||||
}
|
}
|
||||||
|
fcl.deadlineMu.Unlock()
|
||||||
|
|
||||||
// since we're no longer using this listener,
|
// since we're no longer using this listener,
|
||||||
// decrement the usage counter and, if no one
|
// decrement the usage counter and, if no one
|
||||||
|
@ -176,7 +206,7 @@ func (fcl *fakeCloseListener) fakeClosedErr() error {
|
||||||
}
|
}
|
||||||
|
|
||||||
type fakeClosePacketConn struct {
|
type fakeClosePacketConn struct {
|
||||||
closed int32 // accessed atomically - TODO: this needs to be shared across the whole app instance, not to cross instance boundaries... hmmm... see #2658 (still relevant?)
|
closed int32 // accessed atomically
|
||||||
usage *int32 // accessed atomically
|
usage *int32 // accessed atomically
|
||||||
key string
|
key string
|
||||||
net.PacketConn
|
net.PacketConn
|
||||||
|
@ -210,16 +240,25 @@ func (fcpc *fakeClosePacketConn) Close() error {
|
||||||
// socket is actually left open.
|
// socket is actually left open.
|
||||||
var errFakeClosed = fmt.Errorf("listener 'closed' 😉")
|
var errFakeClosed = fmt.Errorf("listener 'closed' 😉")
|
||||||
|
|
||||||
// listenerUsage pairs a net.Listener with a
|
// globalListener keeps global state for a listener
|
||||||
// count of how many servers are using it.
|
// that may be shared by multiple servers. In other
|
||||||
type listenerUsage struct {
|
// words, values in this struct exist only once and
|
||||||
usage int32 // accessed atomically
|
// all other uses of these values point to the ones
|
||||||
ln net.Listener
|
// in this struct. In particular, the usage count
|
||||||
pc net.PacketConn
|
// (how many callers are using the listener), the
|
||||||
|
// actual listener, and synchronization of the
|
||||||
|
// listener's deadline changes are singular, global
|
||||||
|
// values that must not be copied.
|
||||||
|
type globalListener struct {
|
||||||
|
usage int32 // accessed atomically
|
||||||
|
deadline bool
|
||||||
|
deadlineMu sync.Mutex
|
||||||
|
ln net.Listener
|
||||||
|
pc net.PacketConn
|
||||||
}
|
}
|
||||||
|
|
||||||
var (
|
var (
|
||||||
listeners = make(map[string]*listenerUsage)
|
listeners = make(map[string]*globalListener)
|
||||||
listenersMu sync.Mutex
|
listenersMu sync.Mutex
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue