improve Patch utility: add idempotency #12919
Open
+85
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 20m 28s ⏱️ Results for commit ada4ee6. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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