Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

locks trailers during iteration #3595

Merged
merged 1 commit into from
Apr 12, 2021
Merged

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Apr 8, 2021

I was recently debugging a panic in our queriers and found out we acquire a lock for mutating this slice but don't acquire the lock for reading. I'm guessing this was an oversight as the lock was only used in one place prior (the addTrailer method).

panic: runtime error: invalid memory address or nil pointer dereference

goroutine 34388532 [running]:
github.com/grafana/loki/pkg/util/server.onPanic(0x2072aa0, 0x3787ff0, 0x3787ff0, 0x0)
/src/loki/pkg/util/server/recovery.go:41 +0x7b
github.com/grafana/loki/pkg/util/server.glob..func1.1.1(0x2834870, 0xc004b2e8c0)
/src/loki/pkg/util/server/recovery.go:29 +0x55
panic(0x2072aa0, 0x3787ff0)
/usr/local/go/src/runtime/panic.go:965 +0x1b9
github.com/grafana/loki/pkg/logql/stats.decodeTrailer(0x283eea8, 0xc054461d10, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
/src/loki/pkg/logql/stats/grpc.go:115 +0xc8
github.com/grafana/loki/pkg/logql/stats.decodeTrailers(0x283eea8, 0xc054461d10, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
/src/loki/pkg/logql/stats/grpc.go:95 +0x117
github.com/grafana/loki/pkg/logql/stats.Snapshot(0x283eea8, 0xc054461d10, 0x12b0ec7, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
/src/loki/pkg/logql/stats/context.go:149 +0x71
github.com/grafana/loki/pkg/logql.(*query).Exec(0xc02dc7e190, 0x283ee70, 0xc054461d10, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
/src/loki/pkg/logql/engine.go:155 +0x3a5
github.com/grafana/loki/pkg/querier.(*Querier).RangeQueryHandler(0xc000502a10, 0x2834870, 0xc004b2e8c0, 0xc0266f1500)
/src/loki/pkg/querier/http.go:61 +0x362


Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

It wasn't an oversight we never call decode trailers in // as opposed to encode but I don't think it can hurt to have this perf wise.

So careful I wouldn't assume this was the cause of your panic.

@owen-d owen-d merged commit d9637a6 into grafana:master Apr 12, 2021
@owen-d owen-d deleted the sync/lock-trailers branch April 12, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants