Skip to content

Commit

Permalink
sdk/resource: Fix data race with emptyAttributes (#4409)
Browse files Browse the repository at this point in the history
* Fix ASAN bug with emptyAttributes.

If mutliple instantiations of a Resources struct are happening
in parallel, they can end up modifying the same underlying resource.

* Capture literal

* add test

* remove unwanted change

* Update sdk/resource/resource_test.go

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* Update sdk/resource/resource_test.go

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* Update sdk/resource/resource_test.go

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* Update sdk/resource/resource_test.go

Co-authored-by: Robert Pająk <pellared@hotmail.com>

* Add changelog.

* Update CHANGELOG.md

Co-authored-by: Robert Pająk <pellared@hotmail.com>

---------

Co-authored-by: Robert Pająk <pellared@hotmail.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
  • Loading branch information
3 people committed Aug 7, 2023
1 parent e1e6b96 commit b221025
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Decouple `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp/internal` from `go.opentelemetry.io/otel/exporters/otlp/internal` and `go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal` using gotmpl. (#4401, #3846)
- Do not block the metric SDK when OTLP metric exports are blocked in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc` and `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`. (#3925, #4395)
- Do not append _total if the counter already ends in total `go.opentelemetry.io/otel/exporter/prometheus`. (#4373)
- Fix resource detection data race in `go.opentelemetry.io/otel/sdk/resource`. (#4409)

## [1.16.0/0.39.0] 2023-05-18

Expand Down
9 changes: 4 additions & 5 deletions sdk/resource/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ type Resource struct {
}

var (
emptyResource Resource
defaultResource *Resource
defaultResourceOnce sync.Once
)
Expand Down Expand Up @@ -70,7 +69,7 @@ func NewWithAttributes(schemaURL string, attrs ...attribute.KeyValue) *Resource
// of the attrs is known use NewWithAttributes instead.
func NewSchemaless(attrs ...attribute.KeyValue) *Resource {
if len(attrs) == 0 {
return &emptyResource
return &Resource{}
}

// Ensure attributes comply with the specification:
Expand All @@ -81,7 +80,7 @@ func NewSchemaless(attrs ...attribute.KeyValue) *Resource {

// If attrs only contains invalid entries do not allocate a new resource.
if s.Len() == 0 {
return &emptyResource
return &Resource{}
}

return &Resource{attrs: s} //nolint
Expand Down Expand Up @@ -195,7 +194,7 @@ func Merge(a, b *Resource) (*Resource, error) {
// Empty returns an instance of Resource with no attributes. It is
// equivalent to a `nil` Resource.
func Empty() *Resource {
return &emptyResource
return &Resource{}
}

// Default returns an instance of Resource with a default
Expand All @@ -214,7 +213,7 @@ func Default() *Resource {
}
// If Detect did not return a valid resource, fall back to emptyResource.
if defaultResource == nil {
defaultResource = &emptyResource
defaultResource = &Resource{}
}
})
return defaultResource
Expand Down
27 changes: 27 additions & 0 deletions sdk/resource/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"os"
"strings"
"sync"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -769,3 +770,29 @@ func TestWithContainer(t *testing.T) {
string(semconv.ContainerIDKey): fakeContainerID,
}, toMap(res))
}

func TestResourceConcurrentSafe(t *testing.T) {
// Creating Resources should also be free of any data races,
// because Resources are immutable.
var wg sync.WaitGroup
for i := 0; i < 2; i++ {
wg.Add(1)
go func() {
defer wg.Done()
d := &fakeDetector{}
_, err := resource.Detect(context.Background(), d)
assert.NoError(t, err)
}()
}
wg.Wait()
}

type fakeDetector struct{}

func (f fakeDetector) Detect(_ context.Context) (*resource.Resource, error) {
// A bit pedantic, but resource.NewWithAttributes returns an empty Resource when
// no attributes specified. We want to make sure that this is concurrent-safe.
return resource.NewWithAttributes("https://opentelemetry.io/schemas/1.3.0"), nil
}

var _ resource.Detector = &fakeDetector{}

0 comments on commit b221025

Please sign in to comment.