gocheck: serialize log writes

gocheck: serialize log writes

Fixes LP #1084878.

The race detector cannot profile gocheck tested code due to this race.

https://code.launchpad.net/~dave-cheney/gocheck/003-serialise-log-writes/+merge/138378

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-12-06 04:54:23]

Please take a look.

[dave_cheney.net, 2012-12-06 05:00:40]

Please take a look.

[rog, 2012-12-06 06:51:57]

LGTM with a few thoughts below

https://codereview.appspot.com/6862050/diff/5/gocheck.go
File gocheck.go (right):

https://codereview.appspot.com/6862050/diff/5/gocheck.go#newcode620
gocheck.go:620: type serialisedLogger struct {
s/serialisedLogger/logger/
and lose the interface type?

https://codereview.appspot.com/6862050/diff/5/helpers_test.go
File helpers_test.go (right):

https://codereview.appspot.com/6862050/diff/5/helpers_test.go#newcode441
helpers_test.go:441: go func(i int) {
personally i think that:
   i := i
reads more naturally than declaring a parameter and passing it.

https://codereview.appspot.com/6862050/diff/5/helpers_test.go#newcode449
helpers_test.go:449: start.Done()
this seems a slightly inappropriate use of sync.WaitGroup. i'd use a channel for
this and close it to start everything.

[niemeyer, 2012-12-06 11:43:02]

https://codereview.appspot.com/6862050/diff/5/gocheck.go
File gocheck.go (right):

https://codereview.appspot.com/6862050/diff/5/gocheck.go#newcode145
gocheck.go:145: func (c *C) writeLog(buf []byte) {
Isn't it enough to add a lock to this method?

[niemeyer, 2012-12-06 11:47:04]

https://codereview.appspot.com/6862050/diff/5/helpers_test.go
File helpers_test.go (right):

https://codereview.appspot.com/6862050/diff/5/helpers_test.go#newcode449
helpers_test.go:449: start.Done()
On 2012/12/06 06:51:57, rog wrote:
> this seems a slightly inappropriate use of sync.WaitGroup. i'd use a channel
for
> this and close it to start everything.

Why is it inappropriate? It feels very clean and clear to me. Inappropriate != I
haven't seen it before.

[dave_cheney.net, 2012-12-07 01:57:31]

Please take a look.

[dave_cheney.net, 2012-12-10 00:07:39]

Please take a look.

[fwereade, 2012-12-10 07:27:27]

LGTM

[dave_cheney.net, 2012-12-11 00:57:41]

On 2012/12/10 07:27:27, fwereade wrote:
> LGTM

Actually, this doesn't fix all the races. The bytes.Buffer is shared across
loggers recursively. I'm going to go back to an adaptation of patch set #2

[dave_cheney.net, 2012-12-11 02:34:03]

Please take a look.

[rog, 2012-12-11 12:35:13]

On 2012/12/11 02:34:03, dfc wrote:
> Please take a look.

LGTM

[dave_cheney.net, 2012-12-13 05:07:33]

On 2012/12/11 12:35:13, rog wrote:
> On 2012/12/11 02:34:03, dfc wrote:
> > Please take a look.
> 
> LGTM

lbox has screwed up, moving to a new branch.

[dave_cheney.net, 2012-12-13 05:14:13]

Gah - still can't submit,

-----
gocheck: serialize log writes

Fixes LP #1084878.

The race detector cannot profile gocheck tested code due to this race.
-----
Rietveld: https://codereview.appspot.com/6862050
error: Failed to run "bzr merge": exit status 3
-----
bzr: ERROR: Cannot lock
LockDir(chroot-90140432:///%2Bbranch/gocheck/.bzr/branch/lock): Transport
operation not possible: readonly transport

[niemeyer, 2013-03-01 22:49:17]

LGTM

https://codereview.appspot.com/6862050/diff/27001/gocheck.go
File gocheck.go (right):

https://codereview.appspot.com/6862050/diff/27001/gocheck.go#newcode60
gocheck.go:60: writeLock sync.Mutex // protects logb from concurrent writes
That's now unused.

https://codereview.appspot.com/6862050/diff/27001/gocheck.go#newcode74
gocheck.go:74: // logger concurrency safe byte.Buffer
logger is a

https://codereview.appspot.com/6862050/diff/27001/helpers_test.go
File helpers_test.go (right):

https://codereview.appspot.com/6862050/diff/27001/helpers_test.go#newcode450
helpers_test.go:450: stop.Wait()
Shouldn't be hard to have an assertion here checking that there wasn't
corruption.

[niemeyer, 2013-03-01 22:51:36]

*** Submitted:

gocheck: serialize log writes

Fixes LP #1084878.

The race detector cannot profile gocheck tested code due to this race.

R=rog, niemeyer, fwereade
CC=
https://codereview.appspot.com/6862050

[dave_cheney.net, 2013-03-01 23:11:21]

Thanks Gustavo. 

On 02/03/2013, at 9:51, n13m3y3r@gmail.com wrote:

> *** Submitted:
> 
> gocheck: serialize log writes
> 
> Fixes LP #1084878.
> 
> The race detector cannot profile gocheck tested code due to this race.
> 
> R=rog, niemeyer, fwereade
> CC=
> https://codereview.appspot.com/6862050
> 
> 
> https://codereview.appspot.com/6862050/