improve Patch utility: add idempotency #12919
Conversation
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 20m 28s ⏱️ Results for commit ada4ee6. |
There was a problem hiding this comment.
LGTM!
the main behavioral change being the check for is_applied before apply and undo, which makes a lot of sense to me. no idea why i didn't think of adding that originally 🤔 if there was a particular reason i cannot remember it!
just one question around concurrency whether we really need the lock, but i'm fine with adding it.
and sorry for not reviewing sooner!
| @@ -72,6 +73,7 @@ def proxy(*args, **kwargs): | |||
|
|
|||
| class Patch: | |||
| applied_patches: List["Patch"] = [] | |||
| _lock: threading.RLock = threading.RLock() | |||
There was a problem hiding this comment.
q: do we really need thread safety in the Patch object? is it likely that multiple threads will call patch.apply() on the same patch object? or is this to guard the applied_patches data structure?
Motivation
While using the Patch utility, we realize we had to manually check
if patch.is_appliedbefore applying it again because it did not check itself if it was already applied.This PR adds this check internally to avoid applying the patch twice.
It also adds a test for it, and some more patches for some issues we realized and should keep track of.
I've added a single lock for the
Patchclass as we're mutating the global class variableapplied_patches, and it feels like once we start adding more checks for multiple patches mutating the same function, this kind of global lock could be useful.Linearizing the Patching mechanism over one single lock sounds a bit safer than having a single lock per Patch, and will probably end up using a little bit less memory.
Changes
Patch.applyandPatch.undoto verify if the patch was already applied, locked with a single lock for the Patch class