fix(generic-worker): avoid reclaim creds deadlock#8290
Conversation
| taskMount.Infof("Downloading %v to %v", ac, file) | ||
|
|
||
| var runID int64 = -1 // use the latest run | ||
| _, contentLength, err := taskMount.task.Queue.DownloadArtifactToFile(ac.TaskID, runID, ac.Artifact, file) |
There was a problem hiding this comment.
Was previously a data race with the reclaim goroutine that writes taskMount.task.Queue.
| return ResourceUnavailable(e) | ||
| } | ||
|
|
||
| e = artifact.FinishArtifact(resp, task.Queue, task.TaskID, strconv.Itoa(int(task.RunID)), artifact.Base().Name) |
There was a problem hiding this comment.
Was previously a data race with the reclaim goroutine that writes task.Queue.
matt-boris
requested review from
lotas and
petemoore
and removed request for
a team
matt-boris
force-pushed
the
matt-boris/fixReclaimDeadlock
branch
2 times, most recently
from
429959c to
578120b
Compare
|
It's possible we can bump up the concurrent artifact upload goroutines after this lands too, as my previous thoughts look to have been incorrect. I'd like to see how this performs, though first. Relevant previous PR: #8167. |
| par := tcqueue.PostArtifactRequest(json.RawMessage(payload)) | ||
| task.queueMux.RLock() | ||
| parsp, err := task.Queue.CreateArtifact( | ||
| queue := task.Queue |
There was a problem hiding this comment.
is there any benefit of introducing this variable?
There was a problem hiding this comment.
Yes! I'm quickly grabbing the reference to the queue client while holding the lock, then immediately releasing the lock right afterwards. As opposed to holding the lock throughout the entire API call.
lotas
left a comment
lotas
left a comment
There was a problem hiding this comment.
LGTM, but I think Pete should have a closer look :)
2 participants
Fixes #8289.