code review 6782140: log/syslog: retry once if write fails

src/pkg/log/syslog/syslog.go

 // Copyright 2009 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 // +build !windows,!plan9
 
 // Package syslog provides a simple interface to the system log
 // service. It can send messages to the syslog daemon using UNIX
 // domain sockets, UDP, or TCP connections.
 package syslog
 
 import (
         "errors"
         "fmt"
         "log"
         "net"
         "os"
+        "sync"
         "time"
 )
 
 // The Priority is a combination of the syslog facility and
 // severity. For example, LOG_ALERT | LOG_FTP sends an alert severity
 // message from the FTP facility. The default severity is LOG_EMERG;
 // the default facility is LOG_KERN.
 type Priority int
 
 const severityMask = 0x07
@@ skipping 43 matching lines @@
         LOG_LOCAL5
         LOG_LOCAL6
         LOG_LOCAL7
 )
 
 // A Writer is a connection to a syslog server.
 type Writer struct {
         priority Priority
         tag      string
         hostname string
-        conn     serverConn
+        network  string
+        raddr    string
+
+        mu   sync.Mutex // guards conn
+        conn serverConn
 }
 
 type serverConn interface {

[mikio, 2012/12/16 06:54:17]

I think this interface is unnecessary unless you want to
use both net.Conn and net.PacketConn.

[jeff.allen, 2012/12/18 15:18:03]

Done.

         writeString(p Priority, hostname, tag, s string) (int, error)
         close() error
 }
 
 type netConn struct {

[mikio, 2012/12/16 06:54:17]

ditto.

[jeff.allen, 2012/12/18 15:18:03]

Done.

         conn net.Conn
 }
 
 // New establishes a new connection to the system log daemon.  Each
 // write to the returned writer sends a log message with the given
 // priority and prefix.
 func New(priority Priority, tag string) (w *Writer, err error) {
         return Dial("", "", priority, tag)
 }
 
 // Dial establishes a connection to a log daemon by connecting to
 // address raddr on the network net.  Each write to the returned
 // writer sends a log message with the given facility, severity and
 // tag.
-func Dial(network, raddr string, priority Priority, tag string) (w *Writer, err error) {
+func Dial(network, raddr string, priority Priority, tag string) (*Writer, error) {
         if priority < 0 || priority > LOG_LOCAL7|LOG_DEBUG {
                 return nil, errors.New("log/syslog: invalid priority")
         }
 
         if tag == "" {
                 tag = os.Args[0]
         }
-
         hostname, _ := os.Hostname()
 
-        var conn serverConn
-        if network == "" {
-                conn, err = unixSyslog()
-                if hostname == "" {
-                        hostname = "localhost"
+        w := &Writer{
+                priority: priority,
+                tag:      tag,
+                hostname: hostname,
+                network:  network,
+                raddr:    raddr,
+        }
+
+        // w.mu not held, but w is still private to us at this point
+        err := w.connect()
+        if err != nil {
+                return nil, err
+        }
+        return w, err
+}
+
+// (*Writer).connect makes a connection to the syslog server.
+// It must be called with w.mu held.
+func (w *Writer) connect() (err error) {
+        if w.conn != nil {
+                // ignore err from close, it makes sense to continue anyway
+                w.conn.close()
+                w.conn = nil
+        }
+
+        if w.network == "" {
+                w.conn, err = unixSyslog()
+                if w.hostname == "" {
+                        w.hostname = "localhost"
                 }
         } else {
                 var c net.Conn
-                c, err = net.Dial(network, raddr)
-                conn = netConn{c}
-                if hostname == "" {
-                        hostname = c.LocalAddr().String()
+                c, err = net.Dial(w.network, w.raddr)
+                if err == nil {
+                        w.conn = netConn{c}
+                        if w.hostname == "" {
+                                w.hostname = c.LocalAddr().String()
+                        }
                 }
         }
-        if err != nil {
-                return nil, err
-        }
-
-        return &Writer{priority: priority, tag: tag, hostname: hostname, conn: conn}, nil
+        return
 }
 
 // Write sends a log message to the syslog daemon.
 func (w *Writer) Write(b []byte) (int, error) {
         return w.writeString(w.priority, string(b))
 }
 
-func (w *Writer) Close() error { return w.conn.close() }
+func (w *Writer) Close() error {

[mikio, 2012/12/16 06:54:17]

Close is exposed so it needs comments.

[jeff.allen, 2012/12/18 15:18:03]

Done.

+        w.mu.Lock()
+        defer w.mu.Unlock()
+
+        if w.conn != nil {
+                err := w.conn.close()
+                w.conn = nil
+                return err
+        }
+        return nil
+}
 
 // Emerg logs a message with severity LOG_EMERG, ignoring the severity
 // passed to New.
 func (w *Writer) Emerg(m string) (err error) {
         _, err = w.writeString(LOG_EMERG, m)
         return err
 }
 
 // Alert logs a message with severity LOG_ALERT, ignoring the severity
 // passed to New.
@@ skipping 38 matching lines @@
 }
 
 // Debug logs a message with severity LOG_DEBUG, ignoring the severity
 // passed to New.
 func (w *Writer) Debug(m string) (err error) {
         _, err = w.writeString(LOG_DEBUG, m)
         return err
 }
 
 func (w *Writer) writeString(p Priority, s string) (int, error) {
-        return w.conn.writeString((w.priority&facilityMask)|(p&severityMask),
+        w.mu.Lock()
+        defer w.mu.Unlock()
+
+        if w.conn == nil {

[mikio, 2012/12/16 06:54:17]

I'm not keen to manage a conn state with write-only lock because 
the state of conn might be a single point of failure to all syslog.Writer 
users in a single go process. For example, once the conn brings down, 
all users would be blocked by this state until new conn estab attempts 
finished (no DialTimeout...).

I have no clue, do you have any good solution for this?
E.g., anyway syslog.Writer fails as quick as possible and wont block up
users during re-connecting.

[jeff.allen, 2012/12/18 15:18:03]

I think the correct approach to this risk is to reduce the number of callers to
syslog.Write, and the correct way to do that is for the calling application to
queue pending log message itself via a buffering channel. I think the vast
majority of callers don't need this, so I don't think it should be in the
standard library. (For comparison: log.New on an io.Writer which represents a
TCP pipe could cause log.Write to block while holding log.Logger.mu, and all
other callers would block as well.)

So my proposal is to not do anything and let the caller deal with it if they
care. I could add a sentence or two in the docs explaining this.

+                err := w.connect()
+                if err != nil {
+                        return 0, err
+                }
+        }
+        n, err := w.conn.writeString((w.priority&facilityMask)|(p&severityMask),
                 w.hostname, w.tag, s)
+        if err != nil {
+                // try once to reconnect, to handle the case where
+                // the unix socket was closed and reopened by a restarting
+                // syslogd.
+                err = w.connect()
+                if err != nil {
+                        return 0, err
+                }
+                n, err = w.conn.writeString((w.priority&facilityMask)|(p&severityMask),
+                        w.hostname, w.tag, s)
+        }
+        return n, err
 }
 
 // writeString: generates and writes a syslog formatted string. The
 // format is as follows: <PRI>1 TIMESTAMP HOSTNAME TAG[PID]: MSG
 func (n netConn) writeString(p Priority, hostname, tag, msg string) (int, error) {
         nl := ""
         if len(msg) == 0 || msg[len(msg)-1] != '\n' {
                 nl = "\n"
         }
         timestamp := time.Now().Format(time.RFC3339)
@@ skipping 12 matching lines @@
 // the system log service with the specified priority. The logFlag
 // argument is the flag set passed through to log.New to create
 // the Logger.
 func NewLogger(p Priority, logFlag int) (*log.Logger, error) {
         s, err := New(p, "")
         if err != nil {
                 return nil, err
         }
         return log.New(s, "", logFlag), nil
 }