Importer: Do Not Ignore Last Tag Lookup Error When Importing Tags
Context
To import tags concurrently, the importer uses a goroutine to perform parallel tag lookup in order to retrieve the digest associated with the tag combined with a serial for loop which ensures that those tags are represented in the database.
Currently, the importer uses context cancelation within the tag lookup goroutine to stop parallel tag imports when there is an error. The context cancelation takes effect when function are called with the canceled context within the serial loop body.
Problem
If only the last tag to be looked up has an error, then no tagLookupResponse
is sent and the tagResChan
closes, preventing the loop body from executing and for the canceled context to bubble up, stopping the import with an error. Instead, the importer logs error reading tag details, canceling context
and the error message, while the import finishes without reporting any error.
Solutions
tagLookupResponse
Always Send A This solution would be to always sent a tagLookupResponse
on tagResChan
. tagLookupResponse
would be modified to include an error
field that could be checked at the beginning of the serial loop.
Incomplete Example
go func(t string) {
defer func() {
<-semaphore
wg.Done()
}()
desc, err := tagService.Get(ctx, t)
tagResChan <- &tagLookupResponse{t, desc, err)
}(tag)
}
...
for tRes := range tagResChan {
if tRes.error != nil {
...
Pros
- No additional log message to determine the underlying error
- Moves error handling logic outside the goroutine and into normal program execution time.
Cons
- This approach prevents stopping tag detail lookups concurrently. We may perform up to
tagConcurrency - 1
tag lookups before the error is handled.
lookupCtx.Done()
Always Check This solution is to ensure that at the end of the importTags
function after the serial loop body, we always double-check lookupCtx.Done()
to see if lookupCtx
was canceled.
Incomplete Example
for tRes := range tagResChan {
...
}
if lookupCtx.Done() {
return fmt.Errorf("...")
}
Pros
- This is the simplest change
- This preserves the ability to prevent extra tag lookups during the error path
Cons
- Error handling becomes even more spread out through the function body
- This fix is somewhat narrow, only addressing the specific instances of the very last tag having an error with an otherwise empty
tagResChan