diff options
author | Michael Muré <batolettre@gmail.com> | 2022-06-05 15:01:08 +0200 |
---|---|---|
committer | Michael Muré <batolettre@gmail.com> | 2022-06-05 15:13:49 +0200 |
commit | 7348fb9edb68ca9142f5d87673da48cef733b3d3 (patch) | |
tree | 9d4667ecff1f40491d5825e60293e5c85a725104 /bridge/github/export.go | |
parent | 96327c3371ca762d906209c6114092bbf552c0f4 (diff) | |
download | git-bug-7348fb9edb68ca9142f5d87673da48cef733b3d3.tar.gz |
github: fix data race when closing event channel
I believe the issue was twofold:
When done importing, the calling context is likely still valid, so if the output channel is not read enough and reach capacity, some event producer down the line can be blocked trying to send in that channel. When closing it, this send is still trying to proceed, which is illegal in go.
In rateLimitHandlerClient, there was a need to 2 different type of output channel: core.ExportResult and ImportEvent. To do so, the previous code was using a single channel type RateLimitingEvent and a series of goroutines to read/cast/send to the final channel. This could result in more async goroutine being stuck trying to send in an at-capacity channel. Instead, the code now use a simple synchronous callback to directly push to the final output channel. No concurrency needed anymore and the code is simpler.
Any of those fixes could have resolved the data race, but both fixes is more correct.
Diffstat (limited to 'bridge/github/export.go')
-rw-r--r-- | bridge/github/export.go | 15 |
1 files changed, 1 insertions, 14 deletions
diff --git a/bridge/github/export.go b/bridge/github/export.go index 8c40eb74..35d456c2 100644 --- a/bridge/github/export.go +++ b/bridge/github/export.go @@ -486,23 +486,10 @@ func (ge *githubExporter) cacheGithubLabels(ctx context.Context, gc *rateLimitHa } q := labelsQuery{} - // When performing the queries we have to forward rate limiting events to the - // current channel of export results. - events := make(chan RateLimitingEvent) - defer close(events) - go func() { - for e := range events { - select { - case <-ctx.Done(): - return - case ge.out <- core.NewExportRateLimiting(e.msg): - } - } - }() hasNextPage := true for hasNextPage { - if err := gc.queryWithLimitEvents(ctx, &q, variables, events); err != nil { + if err := gc.queryExport(ctx, &q, variables, ge.out); err != nil { return err } |