cache: improve diff ref release logic. #2563
Conversation
|
I'm curious about the impacts of the bug this is fixing. How would it manifest to a user, and what's the severity? |
The bug could result in errors when trying to use diff refs, in which case the only sure-fire remediation would be to prune the cache of the diff ref, so I'd classify it as an important fix for any users of DiffOp. It could occur after a prune is run (manual or automatic), in which case the prune may release the cache entries for the diff ref's lower and upper input even if the diff ref is still lazy, making the diff ref unmountable. Whether that is a common case or not depends heavily on the use case, but I would say it would probably be bound to get hit by most users eventually w/out this fix. Side note, this is different than the problem described in #2557. I found this bug while fixing that one, but wanted to get this out first as it's more severe and not a pre-existing issue. |
|
Thanks - I'll upgrade as soon as this is merged.
|
There was a problem hiding this comment.
Can you add comments so that it is clear where the releases happens.
Eg.
- on additional clones: "released in ...."
- on
createDiffRefcall "createDiffRef takes ownership of the parents refs". (iiuc). Note that on public API these functions should not take ownership but clone internally so that tracking is easier. In private functions like these it can be more optimized with passing ownership.
sipsma
force-pushed
the
diffref-fix
branch
2 times, most recently
from
393e180 to
de4ea73
Compare
Done |
There was a problem hiding this comment.
Sorry for nitpicking here. I think the failure cases (defer) are ok without special comment (unless specifically tricky case). Reader can usually understand the cases that are contained in the same function. It gets confusing when ownership is passed around to another function or retained in a struct and these can benefit from the extra info.
Before this change, the lower and upper parents provided to the cache manager Diff method were not cloned, which resulted in some code paths incorrectly providing them directly as the parents of the returned ref. This meant that if they were released after the call to Diff, the diff ref could become incorrectly invalidated. Now, the lower and upper are cloned and unit test coverage has been added to test that ref release is handled correctly. Signed-off-by: Erik Sipsma <erik@sipsma.dev>
Before this change, the lower and upper parents provided to the cache
manager Diff method were not cloned, which resulted in some code paths
incorrectly providing them directly as the parents of the returned ref.
This meant that if they were released after the call to Diff, the diff
ref could become incorrectly invalidated.
Now, the lower and upper are cloned and unit test coverage has been
added to test that ref release is handled correctly.
Signed-off-by: Erik Sipsma erik@sipsma.dev