fix race condition when updating labels #4861
Conversation
mislav
left a comment
There was a problem hiding this comment.
Wow, thanks for working on this! I just took a cursory glance and it looks great so far.
One question: how does this handle when the set of labels was edited interactively via prompts and multi-selects in the gh issue edit flow?
|
I can change the tests for interactive mode to cover that case. I think it'll have the same functionality since these changes are downstream from where the logic splits b/w interactive and non-interactive. |
|
Oof, I was off about how the values are parsed during interactive mode here. This is going to require some more lines in order to diff what that multi select survey returns vs what labels existed before. That's gonna be necessary in order to know which labels to add and which ones to delete in separate mutations. Shouldn't be too much work — glad to continue working on this. |
mislav
left a comment
There was a problem hiding this comment.
Thanks, this looks great!
Does updateIssue() still run even if the only edits that the user has requested was regarding labels? Can we somehow skip updateIssue firing if there are no other edits?
Finally, can updateIssue() and label operations, if they both need to happen, run concurrently so that the whole operation finishes earlier?
pkg/cmd/pr/shared/editable_http.go
Outdated
| } | ||
| } | ||
| if len(options.Labels.Remove) > 0 { | ||
| removeLabelIds, err := options.Metadata.LabelsToIDs(options.Labels.Remove) |
There was a problem hiding this comment.
addLabels and removeLabels operations are now done sequentially. If they both need to happen, could be do them concurrently? I would typically recommend using golang.org/x/sync/errgroup for this, except for the side-effect that a failed HTTP call would automatically cancel all other requests in the group, which is not what we'd want here.
There was a problem hiding this comment.
I thought about this but ultimately decided not to to avoid doing a partial update in the case that something goes wrong with one of the requests. Seemed better to do an all-or-none update. Idk if there's precedent for doing partial updates in this app. Curious to what your thoughts are about that.
There was a problem hiding this comment.
That's also related to your second question in your first comment above.
There was a problem hiding this comment.
I thought about this but ultimately decided not to to avoid doing a partial update in the case that something goes wrong with one of the requests. Seemed better to do an all-or-none update.
Not sure about what you mean by "partial update", but here is what I was thinking: if I currently do gh issue edit 123 --title newTitle --add-label bug --remove-label feature, this will result in 3 separate API requests:
addLabelsToLabelableremoveLabelsFromLabelableupdateIssue/updatePullRequestwith thetitleparameter
Right now these 3 API requests will be fired sequentially, but since they are not at all dependent on each other, you could fire them concurrently (e.g. via an errgroup.Group) and wait for them all to finish before proceeding. If any of these requests failed, the rest still have a chance of succeeding, and if all of them succeed, the whole edit command would take drastically less time overall. In the error scenario, if that is what you meant by "partial update", I think it's fine: the GitHub API doesn't have a functionality to execute a series of updates in a "transaction", so any GitHub API client risks getting caught in an in-between state whenever we do multiple edits. This is acceptable and shouldn't be an impediment for parallelization.
There was a problem hiding this comment.
Gotcha. Yeah you understood my point and answered my question. Agree parallelization makes sense here.
There was a problem hiding this comment.
I implemented parallelism using a regular WaitGroup. This way all requests are run even if one fails. I took the liberty of changing the signature of UpdateIssue to accept an io.Writer for error output. This means all errors can be printed to stderr.
Update labels using the `addLabelsToLabelable` and `removeLabelsFromLabelable` mutations instead of via the `updateIssue` mutation that replaces the entire set of labels. This prevents the edit operation from clobbering any unseen changes to the list of labels. Co-authored-by: Mislav Marohnić <mislav@github.com>
7c953ea to
db50b54
Compare
There was a problem hiding this comment.
Thank you! I've switched to errgroup.Group after I've found out that the whole group won't get cancelled on first error unless you use errgroup.WithContext. I find the error handling behavior cleaner this way, and it is also acceptable that only the first HTTP error will get reported and the rest get swallowed. I've moved away from these update operations having a writer stream for error output because I don't think lower-level API functionality should be writing to output streams at all.
|
sounds good! Agree on all of that. I really mulled over how to handle errors. Didnt want to change too much of the code and wasn't sure if it'd be preferable to output all errors or just the first! |
Update labels using the addLabelsToLabelable and
removeLabelsFromLabelable mutations instead of via the updateIssue
mutation. This prevents the update from clobbering any unseen changes to
the list of labels. This could happen if the list of labels was updated
after the metadata was fetched, which would cause the local list to be
out-of-date.
Fixes #4835