Skip to content

fix: use a rwlock to avoid concurrent map read and write#210

Merged
tim-mwangi merged 1 commit intomainfrom
concurrent-rw
Feb 24, 2023
Merged

fix: use a rwlock to avoid concurrent map read and write#210
tim-mwangi merged 1 commit intomainfrom
concurrent-rw

Conversation

@tim-mwangi
Copy link
Copy Markdown
Collaborator

Description

Fixing a crash caused by concurrent read and write of map

fatal error: concurrent map read and map write

goroutine 53 [running]:
runtime.throw({0xc1255c?, 0xb66080?})
	/usr/lib/go-1.18/src/runtime/panic.go:992 +0x71 fp=0xc0000bd018 sp=0xc0000bcfe8 pc=0x43aa31
runtime.mapaccess1_faststr(0xb66080?, 0xc000ddb540?, {0xc15193, 0x24})
	/usr/lib/go-1.18/src/runtime/map_faststr.go:22 +0x3a5 fp=0xc0000bd080 sp=0xc0000bd018 pc=0x416265
github.com/hypertrace/goagent/instrumentation/opentelemetry.(*HttpOperationMetricsHandler).AddToRequestCount(0xc0001266d0, 0xc000659f20?, 0xc000ad4f00)
	/root/go/pkg/mod/github.com/hypertrace/goagent@v0.13.0/instrumentation/opentelemetry/metrics.go:56 +0x1d8 fp=0xc0000bd138 sp=0xc0000bd080 pc=0x9fd9b8
github.com/hypertrace/goagent/sdk/instrumentation/net/http.(*handler).ServeHTTP(0xc0001d2410, {0xd10fc0, 0xc0008ec8e0}, 0xc000ad4f00)
	/root/go/pkg/mod/github.com/hypertrace/goagent@v0.13.0/sdk/instrumentation/net/http/handler.go:57 +0xc2 fp=0xc0000bd2c8 sp=0xc0000bd138 pc=0xa16322
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*Handler).ServeHTTP(0xc000cf8900, {0xd10570?, 0xc0001744b0}, 0xc000ad4c00)
	/root/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.39.0/handler.go:213 +0x14b1 fp=0xc0000bd8b0 sp=0xc0000bd2c8 pc=0x9e69f1
main.prometheusMiddleware.func1({0xd107e0?, 0xc000bf69a0}, 0xc000ad4c00)
	/home/ubuntu/go/main.go:114 +0x15c fp=0xc0000bd948 sp=0xc0000bd8b0 pc=0xa6e21c
net/http.HandlerFunc.ServeHTTP(0xc000ad4a00?, {0xd107e0?, 0xc000bf69a0?}, 0x0?)
	/usr/lib/go-1.18/src/net/http/server.go:2084 +0x2f fp=0xc0000bd970 sp=0xc0000bd948 pc=0x6dff0f
github.com/gorilla/mux.(*Router).ServeHTTP(0xc000156000, {0xd107e0, 0xc000bf69a0}, 0xc000ad4900)
	/root/go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 +0x1cf fp=0xc0000bda98 sp=0xc0000bd970 pc=0xa0c74f
net/http.serverHandler.ServeHTTP({0xd0eb30?}, {0xd107e0, 0xc000bf69a0}, 0xc000ad4900)
	/usr/lib/go-1.18/src/net/http/server.go:2916 +0x43b fp=0xc0000bdb58 sp=0xc0000bda98 pc=0x6e34fb
net/http.(*conn).serve(0xc0003ded20, {0xd11a48, 0xc000170cf0})
	/usr/lib/go-1.18/src/net/http/server.go:1966 +0x5d7 fp=0xc0000bdfb8 sp=0xc0000bdb58 pc=0x6de9b7
net/http.(*Server).Serve.func3()
	/usr/lib/go-1.18/src/net/http/server.go:3071 +0x2e fp=0xc0000bdfe0 sp=0xc0000bdfb8 pc=0x6e3e4e
runtime.goexit()
	/usr/lib/go-1.18/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc0000bdfe8 sp=0xc0000bdfe0 pc=0x46a2e1
created by net/http.(*Server).Serve
	/usr/lib/go-1.18/src/net/http/server.go:3071 +0x4db

Testing

Local Tests

Checklist:

  • [ ✅ ] My changes generate no new warnings

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 24, 2023

Codecov Report

Merging #210 (2c4672d) into main (08548ae) will decrease coverage by 0.14%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #210      +/-   ##
==========================================
- Coverage   58.84%   58.71%   -0.14%     
==========================================
  Files          55       55              
  Lines        2243     2248       +5     
==========================================
  Hits         1320     1320              
- Misses        864      869       +5     
  Partials       59       59              
Impacted Files Coverage Δ
instrumentation/opentelemetry/metrics.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

}

mh.countersMutex.Lock()
defer mh.countersMutex.Unlock()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we think AddToRequestCount is racing with CreateRequestCount? But if I look at the code it looks like CreateRequestCount would have had to happen first as part init of WrapHandler?

Also we don't really need a map here right? Because we have a static set of metrics and can precreate counters?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me think about how this is designed and I agree with you. I need to maybe get rid of the map and switch to static counters. I had a map because I thought there would be more counters we would add but we do not need a map if we can reference the counters directly.

@tim-mwangi tim-mwangi merged commit 1afdcd7 into main Feb 24, 2023
@tim-mwangi tim-mwangi deleted the concurrent-rw branch February 24, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants