-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
improve Patch utility: add idempotency #12919
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
Conversation
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 21m 16s ⏱️ Results for commit 302a4fc. ♻️ This comment has been updated with latest results. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Good question 😄 I mentioned it in the PR description, but can maybe go a bit further:
I've added a single lock for the Patch class as we're mutating the global class variable
applied_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.
I was a bit more worried about the case when we start applying different patches to the same function. I also somewhat wanted to protect the applied_patches
datastructure as the operation is not fully atomic, but to be honest, reading it once more, the most "dangerous" place is maybe Patch.function
which will call get_defining_target
which can change when multiple patches are applied. I think it's fine to remove the lock for now, the only calls are append
and remove
, and no logic is actually applied to applied_patches
(we might just end up with duplicate patches, or maybe a ValueError
if we double call undo
from 2 different threads, but that seems unlikely). We can think about adding it again once we fix the failing tests and think about how to support multiple patching 👍 thanks for raising this
ada4ee6
to
302a4fc
Compare
Motivation
While using the Patch utility, we realize we had to manually check
if patch.is_applied
before 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
Patch
class 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.apply
andPatch.undo
to verify if the patch was already applied, locked with a single lock for the Patch class