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
fix race condition when updating labels #4861
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also related to your second question in your first comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
addLabelsToLabelable
removeLabelsFromLabelable
updateIssue
/updatePullRequest
with thetitle
parameter
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Yeah you understood my point and answered my question. Agree parallelization makes sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀🤔🤣
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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