Skip to content

Commit

Permalink
ensure mutexes arent copied
Browse files Browse the repository at this point in the history
  • Loading branch information
quii committed Feb 7, 2019
1 parent 9ba4583 commit 5cb2ee1
Show file tree
Hide file tree
Showing 8 changed files with 58 additions and 7 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ If there's no link, it's not done yet! [why not contribute?](contributing.md)
11. [Concurrency](concurrency.md) - Learn how to write concurrent code to make your software faster.
12. [Select](select.md) - Learn how to synchronise asynchronous processes elegantly.
13. [Reflection](reflection.md) - Learn about reflection
13. [Sync](sync.md) - Learn some functionality from the sync package including `WaitGroup` and `Mutex`

Property-based tests \(todo\)

Expand Down
1 change: 1 addition & 0 deletions SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* [Concurrency](concurrency.md)
* [Select](select.md)
* [Reflection](reflection.md)
* [Sync](sync.md)

## Build an application

Expand Down
2 changes: 2 additions & 0 deletions build.epub.sh
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ docker run -v `pwd`:/source jagregory/pandoc -o learn-go-with-tests.epub --toc -
concurrency.md \
select.md \
reflection.md \
sync.md \
app-intro.md \
http-server.md \
json.md \
io.md \
Expand Down
37 changes: 37 additions & 0 deletions sync.md
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,39 @@ Exposing `Lock` and `Unlock` is at best confusing but at worst potentially very

_This seems like a really bad idea_

## Copying mutexes

Our test passes but our code is still a bit dangerous

If you run `go vet` on your code you should get an error like the following

```
sync/v2/sync_test.go:16: call of assertCounter copies lock value: v1.Counter contains sync.Mutex
sync/v2/sync_test.go:39: assertCounter passes lock by value: v1.Counter contains sync.Mutex
```

A look at the documentation of [`sync.Mutex`](https://golang.org/pkg/sync/#Mutex) tells us why

> A Mutex must not be copied after first use.
When we pass our `Counter` (by value) to `assertCounter` it will try and create a copy of the mutex.

To solve this we should pass in a pointer to our `Counter` instead, so change the signature of `assertCounter`

```go
func assertCounter(t *testing.T, got *Counter, want int)
```

Our tests will no longer compile because we are trying to pass in a `Counter` rather than a `*Counter`. To solve this I prefer to create a constructor which shows readers of your API that it would be better to not initialise the type yourself.

```go
func NewCounter() *Counter {
return &Counter{}
}
```

Use this function in your tests when initialising `Counter`.

## Wrapping up

We've covered a few things from the [sync package](https://golang.org/pkg/sync/)
Expand All @@ -226,3 +259,7 @@ Paraphrasing:

- **Use channels when passing ownership of data**
- **Use mutexes for managing state**

### go vet

Remember to use go vet in your build scripts as it can alert you to some subtle bugs in your code before they hit your poor users.
4 changes: 3 additions & 1 deletion sync/v1/sync.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
package v1


// Counter will increment a number
type Counter struct {
value int
}

// Inc the count
func (c *Counter) Inc() {
c.value++
}

// Value returns the current count
func (c *Counter) Value() int {
return c.value
}
2 changes: 1 addition & 1 deletion sync/v1/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ func TestCounter(t *testing.T) {
})
}

func assertCounter(t *testing.T, got Counter, want int) {
func assertCounter(t *testing.T, got Counter, want int) {
t.Helper()
if got.Value() != want {
t.Errorf("got %d, want %d", got.Value(), want)
Expand Down
10 changes: 9 additions & 1 deletion sync/v2/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,25 @@ package v1

import "sync"

// Counter will increment a number
type Counter struct {
value int
lock sync.Mutex
lock sync.Mutex
}

// NewCounter returns a new Counter
func NewCounter() *Counter {
return &Counter{}
}

// Inc the count
func (c *Counter) Inc() {
c.lock.Lock()
defer c.lock.Unlock()
c.value++
}

// Value returns the current count
func (c *Counter) Value() int {
return c.value
}
8 changes: 4 additions & 4 deletions sync/v2/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
func TestCounter(t *testing.T) {

t.Run("incrementing the counter 3 times leaves it at 3", func(t *testing.T) {
counter := Counter{}
counter := NewCounter()
counter.Inc()
counter.Inc()
counter.Inc()
Expand All @@ -18,12 +18,12 @@ func TestCounter(t *testing.T) {

t.Run("it runs safely concurrently", func(t *testing.T) {
wantedCount := 1000
counter := Counter{}
counter := NewCounter()

var wg sync.WaitGroup
wg.Add(wantedCount)

for i:=0; i<wantedCount; i++ {
for i := 0; i < wantedCount; i++ {
go func(w *sync.WaitGroup) {
counter.Inc()
w.Done()
Expand All @@ -36,7 +36,7 @@ func TestCounter(t *testing.T) {

}

func assertCounter(t *testing.T, got Counter, want int) {
func assertCounter(t *testing.T, got *Counter, want int) {
t.Helper()
if got.Value() != want {
t.Errorf("got %d, want %d", got.Value(), want)
Expand Down

0 comments on commit 5cb2ee1

Please sign in to comment.