plumbing: object, fix Change.PatchContext hardcoded 1-hour internal timeout #1910
plumbing: object, fix Change.PatchContext hardcoded 1-hour internal timeout #1910bitpeng wants to merge 1 commit intogo-git:mainfrom
Conversation
AriehSchneier
left a comment
There was a problem hiding this comment.
The direction is right — diff.Do has a hardcoded 1-hour internal timeout that completely ignores any shorter context deadline, so this is a real bug worth fixing. A few things to address:
Also consider adding an early cancellation check at the top of filePatchWithContext, before the expensive fileContent reads:
if err := ctx.Err(); err != nil {
return nil, ErrCanceled
}This avoids reading file content and running the diff entirely if the context is already done on entry.
A test covering the new codepath would also be valuable — e.g. pass a context with a 1ms deadline and verify the function returns promptly rather than running for up to an hour.
| // Choose the appropriate diff method based on the remaining timeout duration. | ||
| // If there's no remaining time (or it's negative), use the regular diff.Do. | ||
| // Otherwise, use diff.DoWithTimeout with the calculated remaining time. | ||
| if remaining <= 0 { |
There was a problem hiding this comment.
If remaining <= 0 the deadline has already passed — falling back to diff.Do here means the function will run for up to an hour on an already-expired context. Should return early instead:
if !ok {
diffs = diff.Do(fromContent, toContent)
} else if remaining <= 0 {
return nil, ErrCanceled
} else {
diffs = diff.DoWithTimeout(fromContent, toContent, remaining)
}| if remaining <= 0 { | ||
| diffs = diff.Do(fromContent, toContent) | ||
| } else { | ||
| diffs = diff.DoWithTimeout(fromContent, toContent, remaining) |
There was a problem hiding this comment.
Worth adding a comment: diff.DoWithTimeout silently returns a best-effort (potentially incomplete) diff when its internal timeout fires — it doesn't surface an error. The ctx.Done() check in the loop below won't reliably catch this. This is a limitation of the go-diff API, but worth documenting so callers aren't surprised.
|
Could you please review the latest commit? Also, should I squash the commits? @AriehSchneier |
Looks good now. You will need to squash. I still suggest adding a test. |
2dd6356 to
86e8452
Compare
|
The commits have been squashed. Apologies, there was no existing test function for filePatchWithContext. As I'm not very familiar with go-git's test suite, it's difficult for me to add appropriate test cases for this commit in a short time. |
|
Not sure it tests all the new paths, but maybe something like: |
…with a ctx that has deadline
86e8452 to
c2fc8a1
Compare
|
Thanks, I've added the test cases as you suggested. Could you please re-review this at your convenience? |
There is a hidden bug in the Change.PatchContext method: even when users call this function with a ctx that has a timeout of only a few seconds, filePatchWithContext always uses diff.Do(fromContent, toContent) — which has a one-hour execution timeout. This is extremely confusing for users.