Repository: go-kit/kit
Pull request #1007 (2020-08-24 13:18:46)

metrics/generic: fix uint64 alignment
Fixes uint64 alignment on 32-bit arch.

- https://golang.org/pkg/sync/atomic/#pkg-note-BUG
- https://github.com/golang/go/issues/599

<details>
<summary>panic on Counter</summary>

```
Stack: goroutine 59 [running]: 
runtime/debug.Stack(0x4d20140, 0x2662937, 0x17)
        /usr/local/go/src/runtime/debug/stack.go:24 +0x78
github.com/containous/traefik/v2/pkg/safe.defaultRecoverGoroutine(0x2232840, 0x41a7570)
        /go/src/github.com/containous/traefik/pkg/safe/routine.go:66 +0x90
github.com/containous/traefik/v2/pkg/safe.GoWithRecover.func1.1(0x276cc34)
        /go/src/github.com/containous/traefik/pkg/safe/routine.go:56 +0x44
panic(0x2232840, 0x41a7570)
        /usr/local/go/src/runtime/panic.go:969 +0x118
runtime/internal/atomic.goLoad64(0x4fbab54, 0x51b6920, 0x2350f01)
        /usr/local/go/src/runtime/internal/atomic/atomic_arm.go:131 +0x1c
github.com/go-kit/kit/metrics/generic.(*Counter).Add(0x4fbab40, 0x0, 0x3ff00000)
        /go/pkg/mod/github.com/go-kit/kit@v0.9.0/metrics/generic/generic.go:46 +0x28
github.com/containous/traefik/v2/pkg/metrics.(*pilotCounter).Add(0x4c0ea70, 0x0, 0x3ff00000)
        /go/src/github.com/containous/traefik/pkg/metrics/pilot.go:229 +0xa4
github.com/go-kit/kit/metrics/multi.Counter.Add(0x4d11940, 0x2, 0x2, 0x0, 0x3ff00000)
        /go/pkg/mod/github.com/go-kit/kit@v0.9.0/metrics/multi/multi.go:20 +0x40
main.setupServer.func3(0x4fcd810, 0x51b68f8, 0x51b6900, 0x51a1f00)
        /go/src/github.com/containous/traefik/cmd/traefik/traefik.go:258 +0x4c
github.com/containous/traefik/v2/pkg/server.(*ConfigurationWatcher).loadMessage(0x51265a0, 0x2626245, 0x4, 0x4fcd710)
        /go/src/github.com/containous/traefik/pkg/server/configurationwatcher.go:150 +0x14c
github.com/containous/traefik/v2/pkg/server.(*ConfigurationWatcher).listenConfigurations(0x51265a0, 0x2ae2d18, 0x4fba0c0)
        /go/src/github.com/containous/traefik/pkg/server/configurationwatcher.go:132 +0x2c
github.com/containous/traefik/v2/pkg/safe.(*Pool).GoCtx.func1()
        /go/src/github.com/containous/traefik/pkg/safe/routine.go:36 +0x5c
github.com/containous/traefik/v2/pkg/safe.GoWithRecover.func1(0x276cc34, 0x51c8010)
        /go/src/github.com/containous/traefik/pkg/safe/routine.go:59 +0x48
created by github.com/containous/traefik/v2/pkg/safe.GoWithRecover
        /go/src/github.com/containous/traefik/pkg/safe/routine.go:53 +0x34
```

</details>

<details>
<summary>panic on Gauge</summary>


```
runtime/debug.Stack(0x44ae140, 0x2653087, 0x17)
	/usr/local/go/src/runtime/debug/stack.go:24 +0x78
github.com/containous/traefik/v2/pkg/safe.defaultRecoverGoroutine(0x2222e48, 0x3e24e78)
	/go/src/github.com/containous/traefik/pkg/safe/routine.go:66 +0x90
github.com/containous/traefik/v2/pkg/safe.GoWithRecover.func1.1(0x275c22c)
	/go/src/github.com/containous/traefik/pkg/safe/routine.go:56 +0x44
panic(0x2222e48, 0x3e24e78)
	/usr/local/go/src/runtime/panic.go:969 +0x118
github.com/go-kit/kit/metrics/generic.(*Gauge).Set(...)
	/go/src/github.com/go-kit/kit/metrics/generic/generic.go:107
github.com/containous/traefik/v2/pkg/metrics.(*pilotGauge).Set(0x440e680, 0x79400000, 0x41d7d0ed)
	/go/src/github.com/containous/traefik/pkg/metrics/pilot.go:261 +0xbc
github.com/go-kit/kit/metrics/multi.Gauge.Set(0x440e780, 0x1, 0x1, 0x79400000, 0x41d7d0ed)
	/go/src/github.com/go-kit/kit/metrics/multi/multi.go:44 +0x40
main.setupServer.func3(0x47de1f0, 0x4713bb8, 0x4713bc0, 0x47dc7a0)
	/go/src/github.com/containous/traefik/cmd/traefik/traefik.go:259 +0xf0
github.com/containous/traefik/v2/pkg/server.(*ConfigurationWatcher).loadMessage(0x488a360, 0x2620f52, 0x8, 0x47de1a0)
	/go/src/github.com/containous/traefik/pkg/server/configurationwatcher.go:150 +0x14c
github.com/containous/traefik/v2/pkg/server.(*ConfigurationWatcher).listenConfigurations(0x488a360, 0x2ad1dc0, 0x46f28a0)
	/go/src/github.com/containous/traefik/pkg/server/configurationwatcher.go:132 +0x2c
github.com/containous/traefik/v2/pkg/safe.(*Pool).GoCtx.func1()
	/go/src/github.com/containous/traefik/pkg/safe/routine.go:36 +0x5c
github.com/containous/traefik/v2/pkg/safe.GoWithRecover.func1(0x275c22c, 0x4605ae0)
	/go/src/github.com/containous/traefik/pkg/safe/routine.go:59 +0x48
created by github.com/containous/traefik/v2/pkg/safe.GoWithRecover
	/go/src/github.com/containous/traefik/pkg/safe/routine.go:53 +0x34 
```

</details>

Related to https://github.com/containous/traefik/issues/7207

peterbourgon: Sure! Please add a test :)
ldez: :thinking: sure but could you help me?

- how to add a test to check the memory alignment without `unsafe`?
- how to add a test to check the memory alignment in a 32-bit arch from a 64-bit arch?

ldez: @peterbourgon friendly ping :wink: 
peterbourgon: Sorry, I don't know offhand!

On Fri, Sep 4, 2020 at 11:11 AM Ludovic Fernandez <notifications@github.com>
wrote:

> @peterbourgon <https://github.com/peterbourgon> friendly ping 😉
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/go-kit/kit/pull/1007#issuecomment-687026879>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAA36TLOD4F6AMFECL45YSLSECVMVANCNFSM4QJPZKTA>
> .
>

ldez: Without a 32bit arch it's not possible to create a simple test about alignment problem on a 32bits arch.
The problem of alignment appears at runtime, not at the compile time.

I see only one solution: be able to run the tests of the project on a CI that use a 32bit arch.
The GitHub Action only provide 64bit arch.

In all cases, this kind of change of the CI seems out of the scope of my PR.

---

As a kind of prof, I will give you some dumps.

## On a 32bit arch

Created from a PI2 (arm)

### Previous version:

```
generic.Counter:
 Name at offset 0, size=8, align=4
 lvs at offset 8, size=12, align=4
 bits at offset 20, size=8, align=4
```

the `bits` offset is 20, but 20 is not a multiple of 8.

### My version:

```
generic.Counter:
 bits at offset 0, size=8, align=4
 Name at offset 8, size=8, align=4
 lvs at offset 16, size=12, align=4
```

## On a 64bit arch

### Previous version:

```
generic.Counter:
 Name at offset 0, size=16, align=8
 lvs at offset 16, size=24, align=8
 bits at offset 40, size=8, align=8
```

### My version:

```
generic.Counter:
 bits at offset 0, size=8, align=8
 Name at offset 8, size=16, align=8
 lvs at offset 24, size=24, align=8
```


peterbourgon: > In all cases, this kind of change of the CI seems out of the scope of my PR.

Well, it's important to the long-term maintainability of the project that changes come with regression tests, so I don't accidentally undo your work in a future refactor. Can you think of any other way to accomplish that?
ldez: I put a comment on the structure to explain why the field should be placed at the top of the structure.
It's a complex problem without simple solution, except if you are able to use the 32bit arch to run the tests.
ldez: I added a test, but it's a weak test, because the real solution is to run the tests on a 32-bit arch.

FYI even staticcheck is not able to catch this misalignment properly without a 32-bit arch.


Patch:
diff --git a/metrics/generic/generic.go b/metrics/generic/generic.go
index ebde9c8..b693345 100644
--- a/metrics/generic/generic.go
+++ b/metrics/generic/generic.go
@@ -18,9 +18,9 @@ import (
 
 // Counter is an in-memory implementation of a Counter.
 type Counter struct {
+	bits uint64 // bits has to be the first word in order to be 64-aligned on 32-bit
 	Name string
 	lvs  lv.LabelValues
-	bits uint64
 }
 
 // NewCounter returns a new, usable Counter.
@@ -81,9 +81,9 @@ func (c *Counter) LabelValues() []string {
 
 // Gauge is an in-memory implementation of a Gauge.
 type Gauge struct {
+	bits uint64 // bits has to be the first word in order to be 64-aligned on 32-bit
 	Name string
 	lvs  lv.LabelValues
-	bits uint64
 }
 
 // NewGauge returns a new, usable Gauge.
diff --git a/sd/etcdv3/client.go b/sd/etcdv3/client.go
index ffc86fe..0ccb9e6 100644
--- a/sd/etcdv3/client.go
+++ b/sd/etcdv3/client.go
@@ -8,6 +8,7 @@ import (
 
 	"go.etcd.io/etcd/clientv3"
 	"go.etcd.io/etcd/pkg/transport"
+	"google.golang.org/grpc"
 )
 
 var (
@@ -73,8 +74,14 @@ type ClientOptions struct {
 	CACert        string
 	DialTimeout   time.Duration
 	DialKeepAlive time.Duration
-	Username      string
-	Password      string
+
+	// DialOptions is a list of dial options for the gRPC client (e.g., for interceptors).
+	// For example, pass grpc.WithBlock() to block until the underlying connection is up.
+	// Without this, Dial returns immediately and connecting the server happens in background.
+	DialOptions []grpc.DialOption
+
+	Username string
+	Password string
 }
 
 // NewClient returns Client with a connection to the named machines. It will
@@ -107,6 +114,7 @@ func NewClient(ctx context.Context, machines []string, options ClientOptions) (C
 		Endpoints:         machines,
 		DialTimeout:       options.DialTimeout,
 		DialKeepAliveTime: options.DialKeepAlive,
+		DialOptions:       options.DialOptions,
 		TLS:               tlscfg,
 		Username:          options.Username,
 		Password:          options.Password,


Test patch:
diff --git a/metrics/generic/generic_test.go b/metrics/generic/generic_test.go
index cb725e0..435f0fc 100644
--- a/metrics/generic/generic_test.go
+++ b/metrics/generic/generic_test.go
@@ -5,6 +5,12 @@ package generic_test
 // generic to use its Histogram in the Quantiles helper function.
 
 import (
+	"go/ast"
+	"go/importer"
+	"go/parser"
+	"go/token"
+	"go/types"
+	"io/ioutil"
 	"math"
 	"math/rand"
 	"sync"
@@ -107,3 +113,72 @@ func TestSimpleHistogram(t *testing.T) {
 		t.Errorf("want %f, have %f", want, have)
 	}
 }
+
+// Naive atomic alignment test.
+// The problem is related to the use of `atomic.*` and not directly to a structure.
+// But currently works for Counter and Gauge.
+// To have a more solid test, this test should be removed and the other tests should be run on a 32-bit arch.
+func TestAtomicAlignment(t *testing.T) {
+	content, err := ioutil.ReadFile("./generic.go")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	fset := token.NewFileSet()
+
+	file, err := parser.ParseFile(fset, "generic.go", content, parser.ParseComments)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	conf := types.Config{Importer: importer.ForCompiler(fset, "source", nil)}
+
+	pkg, err := conf.Check(".", fset, []*ast.File{file}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// uses ARM as reference for 32-bit arch
+	sizes := types.SizesFor("gc", "arm")
+
+	names := []string{"Counter", "Gauge"}
+
+	for _, name := range names {
+		t.Run(name, func(t *testing.T) {
+			checkAtomicAlignment(t, sizes, pkg.Scope().Lookup(name), pkg)
+		})
+	}
+}
+
+func checkAtomicAlignment(t *testing.T, sizes types.Sizes, obj types.Object, pkg *types.Package) {
+	t.Helper()
+
+	st := obj.Type().Underlying().(*types.Struct)
+
+	posToCheck := make(map[int]types.Type)
+
+	var vars []*types.Var
+	for i := 0; i < st.NumFields(); i++ {
+		field := st.Field(i)
+
+		if v, ok := field.Type().(*types.Basic); ok {
+			switch v.Kind() {
+			case types.Uint64, types.Float64, types.Int64:
+				posToCheck[i] = v
+			}
+		}
+
+		vars = append(vars, types.NewVar(field.Pos(), pkg, field.Name(), field.Type()))
+	}
+
+	offsets := sizes.Offsetsof(vars)
+	for i, offset := range offsets {
+		if _, ok := posToCheck[i]; !ok {
+			continue
+		}
+
+		if offset%8 != 0 {
+			t.Errorf("misalignment detected in %s for the type %s, offset %d", obj.Name(), posToCheck[i], offset)
+		}
+	}
+}
diff --git a/sd/etcdv3/client_test.go b/sd/etcdv3/client_test.go
new file mode 100644
index 0000000..ff29a73
--- /dev/null
+++ b/sd/etcdv3/client_test.go
@@ -0,0 +1,80 @@
+package etcdv3
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"google.golang.org/grpc"
+)
+
+const (
+	// irrelevantEndpoint is an address which does not exists.
+	irrelevantEndpoint = "http://irrelevant:12345"
+)
+
+func TestNewClient(t *testing.T) {
+	client, err := NewClient(
+		context.Background(),
+		[]string{irrelevantEndpoint},
+		ClientOptions{
+			DialTimeout:   3 * time.Second,
+			DialKeepAlive: 3 * time.Second,
+		},
+	)
+	if err != nil {
+		t.Fatalf("unexpected error creating client: %v", err)
+	}
+	if client == nil {
+		t.Fatal("expected new Client, got nil")
+	}
+}
+
+func TestClientOptions(t *testing.T) {
+	client, err := NewClient(
+		context.Background(),
+		[]string{},
+		ClientOptions{
+			Cert:          "",
+			Key:           "",
+			CACert:        "",
+			DialTimeout:   3 * time.Second,
+			DialKeepAlive: 3 * time.Second,
+		},
+	)
+	if err == nil {
+		t.Errorf("expected error: %v", err)
+	}
+	if client != nil {
+		t.Fatalf("expected client to be nil on failure")
+	}
+
+	_, err = NewClient(
+		context.Background(),
+		[]string{irrelevantEndpoint},
+		ClientOptions{
+			Cert:          "does-not-exist.crt",
+			Key:           "does-not-exist.key",
+			CACert:        "does-not-exist.CACert",
+			DialTimeout:   3 * time.Second,
+			DialKeepAlive: 3 * time.Second,
+		},
+	)
+	if err == nil {
+		t.Errorf("expected error: %v", err)
+	}
+
+	client, err = NewClient(
+		context.Background(),
+		[]string{irrelevantEndpoint},
+		ClientOptions{
+			DialOptions: []grpc.DialOption{grpc.WithBlock()},
+		},
+	)
+	if err == nil {
+		t.Errorf("expected connection should fail")
+	}
+	if client != nil {
+		t.Errorf("expected client to be nil on failure")
+	}
+}
diff --git a/sd/etcdv3/example_test.go b/sd/etcdv3/example_test.go
index 1422e31..8acf1d3 100644
--- a/sd/etcdv3/example_test.go
+++ b/sd/etcdv3/example_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/go-kit/kit/log"
 	"github.com/go-kit/kit/sd"
 	"github.com/go-kit/kit/sd/lb"
+	"google.golang.org/grpc"
 )
 
 func Example() {
@@ -44,6 +45,9 @@ func Example() {
 
 		// If DialKeepAlive is 0, it defaults to 3s
 		DialKeepAlive: time.Second * 3,
+
+		// If passing `grpc.WithBlock`, dial connection will block until success.
+		DialOptions: []grpc.DialOption{grpc.WithBlock()},
 	}
 
 	// Build the client.