Fix a potential defer logic error around locking #17022
Conversation
in several top-level API functions. These are the first line of the function that contains them, which makes sense; we want to capture any error returned by the function. However, making this the first defer means that it is the last thing to run after the function returns - meaning that the container's `defer c.lock.Unlock()` has already fired, leading to a chance we modify the container without holding its lock. We could move the function around so it's no longer the first defer, but then we'd have to call it twice (immediately after `defer c.lock.Unlock()` if the container is not batched, and a second time in a new `else` block right after the lock/sync call to make sure we handle batched containers). Seems simpler to just leave it like this. [NO NEW TESTS NEEDED] Can't really test for DB corruption easily. Signed-off-by: Matthew Heon <[email protected]>
openshift-ci
bot
added
release-note-none
approved
|
LGTM |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/cherry-pick v4.4 |
|
@baude: new pull request could not be created: failed to create pull request against containers/podman#v4.4 from head openshift-cherrypick-robot:cherry-pick-17022-to-v4.4: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"No commits between containers:v4.4 and openshift-cherrypick-robot:cherry-pick-17022-to-v4.4"}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"} In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
LGTM
@edsantiago this may resolve some of the flakes. I am unsure but in case the flake rate drops, we can send Matt some flowers.
github-actions
bot
added
the
locked - please file new issue/PR
in several top-level API functions. These are the first line of the function that contains them, which makes sense; we want to capture any error returned by the function. However, making this the first defer means that it is the last thing to run after the function returns - meaning that the container's
defer c.lock.Unlock()has already fired, leading to a chance we modify the container without holding its lock.We could move the function around so it's no longer the first defer, but then we'd have to call it twice (immediately after
defer c.lock.Unlock()if the container is not batched, and a second time in a newelseblock right after the lock/sync call to make sure we handle batched containers). Seems simpler to just leave it like this.[NO NEW TESTS NEEDED] Can't really test for DB corruption easily.
Signed-off-by: Matthew Heon [email protected]
Does this PR introduce a user-facing change?