[release/1.7] Fix various timing issues with docker pusher #9921
Conversation
This file contains 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
io.Pipe produces a PipeReader and a PipeWriter - a close on the write side, causes an error on both the read and write sides, while a close on the read side causes an error on only the read side. Previously, we explicitly prohibited closing from the read side. However, http.Request.Body requires that "calling Close should unblock a Read waiting for input". Our reader will not do this - calling close becomes a no-op. This can cause a deadlock because client.Do may never terminate in some circumstances. We need the Reader side to close its side of the pipe as well, which it already does using the go standard library - otherwise, we can hang forever, writing to a pipe that will never be closed. Allowing the requester to close the body should be safe - we never reuse the same reader between requests, as the result of body() will never be reused by the guarantees of the standard library. Signed-off-by: Justin Chadwell <me@jedevc.com>
If Close is called externally before a request is attempted, then we will accidentally attempt to send to a closed channel, causing a panic. To avoid this, we can check to see if Close has been called, using a done channel. If this channel is ever done, we drop any incoming errors, requests or pipes - we don't need them, since we're done. Signed-off-by: Justin Chadwell <me@jedevc.com>
If we get io.ErrClosedPipe in pushWriter.Write, there are three possible scenarios: - The request has failed, we need to attempt a reset, so we can expect a new pipe incoming on pipeC. - The request has failed, we don't need to attempt a reset, so we can expect an incoming error on errC. - Something else externally has called Close, so we can expect the done channel to be closed. This patch ensures that we block for as long as possible (while still handling each of the above cases, so we avoid hanging), to make sure that we properly return an appropriate error message each time. Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
If a writer continually asks to be reset then it should always succeed - it should be the responsibility of the underlying content.Writer to stop producing ErrReset after some amount of time and to instead return the underlying issue - which pushWriter already does today, using the doWithRetries function. doWithRetries already has a separate cap for retries of 6 requests (5 retries after the original failure), and it seems like this would be previously overridden by content.Copy's max number of 5 attempts, hiding the original error. Signed-off-by: Justin Chadwell <me@jedevc.com>
If sending two messages from goroutine X:
a <- 1
b <- 2
And receiving them in goroutine Y:
select {
case <- a:
case <- b:
}
Either branch of the select can trigger first - so when we call
.setError and .Close next to each other, we don't know whether the done
channel will close first or the error channel will receive first - so
sometimes, we get an incorrect error message.
We resolve this by not sending both signals - instead, we can have
.setError *imply* .Close, by having the pushWriter call .Close on
itself, after receiving an error.
Signed-off-by: Justin Chadwell <me@jedevc.com>
We also need an additional check to avoid setting both the error and response which can create a race where they can arrive in the receiving thread in either order. If we hit an error, we don't need to send the response. > There is a condition where the registry (unexpectedly, not to spec) > returns 201 or 204 on the put before the body is fully written. I would > expect that the http library would issue close and could fall into a > deadlock here. We could just read respC and call setResponse. In that > case ErrClosedPipe would get returned and Commit shouldn't be called > anyway. Signed-off-by: Justin Chadwell <me@jedevc.com>
|
Hi @jedevc. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
3 tasks
estesp
approved these changes
Mar 4, 2024
dmcgowan
approved these changes
Mar 4, 2024
dmcgowan
changed the title
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.
Backport of #8379, cc @dmcgowan.
It would be nice to be able to grab this update downstream for buildkit/buildx, see docker/buildx#2232 (comment).