code review 3300042: log: ensure writes are atomic.

log: ensure writes are atomic.
This means that any Writer can be used safely
even when Output is called concurrently.
Fixes issue 1302.

[rog, 2010-11-26 08:41:26]

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

I'd like you to review this change.

[adg, 2010-11-26 10:26:42]

r is a better choice for reviewer than I. It's his code.

Andrew

On 26 November 2010 17:41,  <rogpeppe@gmail.com> wrote:
> Reviewers: adg,
>
> Message:
> Hello adg (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> log: ensure writes are atomic.
> This means that any Writer can be used safely
> even when Output is called concurrently.
> Fixes an issue that I mentioned on the list but
> has no number yet because the issue tracker is down.
>
> Please review this at http://codereview.appspot.com/3300042/
>
> Affected files:
>  M src/pkg/log/log.go
>
>
> Index: src/pkg/log/log.go
> ===================================================================
> --- a/src/pkg/log/log.go
> +++ b/src/pkg/log/log.go
> @@ -19,6 +19,7 @@
>        "runtime"
>        "os"
>        "time"
> +       "sync"
>  )
>
>  // These flags define which text to prefix to each log entry generated by
> the Logger.
> @@ -36,9 +37,10 @@
>
>  // Logger represents an active logging object.
>  type Logger struct {
> -       out    io.Writer // destination for output
> -       prefix string    // prefix to write at beginning of each line
> -       flag   int       // properties
> +       mu     sync.Mutex // ensures atomic writes
> +       out    io.Writer  // destination for output
> +       prefix string     // prefix to write at beginning of each line
> +       flag   int        // properties
>  }
>
>  // New creates a new Logger.   The out variable sets the
> @@ -46,7 +48,7 @@
>  // The prefix appears at the beginning of each generated log line.
>  // The flag argument defines the logging properties.
>  func New(out io.Writer, prefix string, flag int) *Logger {
> -       return &Logger{out, prefix, flag}
> +       return &Logger{out: out, prefix: prefix, flag: flag}
>  }
>
>  var std = New(os.Stderr, "", Ldate|Ltime)
> @@ -139,7 +141,9 @@
>        if len(s) > 0 && s[len(s)-1] != '\n' {
>                buf.WriteByte('\n')
>        }
> +       l.mu.Lock()
>        _, err := l.out.Write(buf.Bytes())
> +       l.mu.Unlock()
>        return err
>  }
>
>
>
>

[rog, 2010-11-26 10:35:28]

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

Please take another look.

[r, 2010-11-26 17:11:38]

See comments in issue tracker. I'd prefer not to do this.

[rog, 2010-11-26 17:38:21]

i've replied there.

On 26 November 2010 17:11, Rob 'Commander' Pike <r@golang.org> wrote:
> See comments in issue tracker. I'd prefer not to do this.
>

[nigeltao_gnome, 2010-11-26 23:22:08]

On 27 November 2010 04:11, Rob 'Commander' Pike <r@golang.org> wrote:
> See comments in issue tracker. I'd prefer not to do this.

For the mailing list, this is issue 1302:
http://code.google.com/p/go/issues/detail?id=1302

[r, 2010-11-29 20:52:10]

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go
File src/pkg/log/log.go (right):

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go#newcode38
src/pkg/log/log.go:38: // Logger represents an active logging object.
A Logger represents an active logging object that generates lines of output to
an io.Writer.  Each logging operation makes a single call to the Writer's Write
method.  A Logger can be used simultaneously from multiple goroutines; it
guarantees to serialize access to the Writer.

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go#newcode144
src/pkg/log/log.go:144: l.mu.Lock()
defer l.mu.Unlock()
(must unlock if write panics)

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go#newcode146
src/pkg/log/log.go:146: l.mu.Unlock()
d

[rog, 2010-12-01 11:33:18]

PTAL

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go
File src/pkg/log/log.go (right):

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go#newcode38
src/pkg/log/log.go:38: // Logger represents an active logging object.
On 2010/11/29 20:52:10, r wrote:
> A Logger represents an active logging object that generates lines of output to
> an io.Writer.  Each logging operation makes a single call to the Writer's
Write
> method.  A Logger can be used simultaneously from multiple goroutines; it
> guarantees to serialize access to the Writer.

Done.

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go#newcode144
src/pkg/log/log.go:144: l.mu.Lock()
On 2010/11/29 20:52:10, r wrote:
> defer l.mu.Unlock()
> (must unlock if write panics)

Done.

http://codereview.appspot.com/3300042/diff/7001/src/pkg/log/log.go#newcode146
src/pkg/log/log.go:146: l.mu.Unlock()
On 2010/11/29 20:52:10, r wrote:
> d

Done.

[r, 2010-12-01 18:15:12]

LGTM

[r, 2010-12-01 18:15:47]

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

log: ensure writes are atomic.
This means that any Writer can be used safely
even when Output is called concurrently.
Fixes issue 1302.

R=r, nigeltao_gnome
CC=golang-dev
http://codereview.appspot.com/3300042

Committer: Rob Pike <r@golang.org>