chunked: add check for more parts #2160
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe 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 |
validate that there are no other parts returned by the client, and do not ignore other errors that could have happened later. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
chunked-consume-channels
branch
from
75717e2 to
cf619cf
Compare
| @@ -1176,6 +1202,10 @@ func (c *chunkedDiffer) copyAllBlobToFile(destination *os.File) (digest.Digest, | |||
| // copy the entire tarball and compute its digest | |||
| _, err = io.CopyBuffer(destination, r, c.copyBuffer) | |||
There was a problem hiding this comment.
Aren't we ignoring this error now?
| err = err1 | ||
| } | ||
| } | ||
| if closed == 2 { |
There was a problem hiding this comment.
I am confused why we have this counter instead of checking err and err1
There was a problem hiding this comment.
I think this doesn’t quite work as is, and https://issues.redhat.com/browse/OCPBUGS-43968 suggests that we need either a redesign or a much more careful error handling.
Also, unless the code becomes somehow trivial, I think the consumption of (streams, errs), starting with the first select already, should be strongly centralized in a helper, not copy&pasted. (We do have some helpers, but those are already copy&pasted instead of used universally.)
| select { | ||
| case _, ok := <-streams: | ||
| if !ok { | ||
| closed++ |
There was a problem hiding this comment.
AFAICS once this happens, this case will always trigger and this function will consume 100% of a CPU until the other channel is closed.
There was a problem hiding this comment.
… and close will keep incrementing.
| case _, ok := <-streams: | ||
| if !ok { | ||
| closed++ | ||
| } else if err == nil { |
There was a problem hiding this comment.
Nothing guarantees that we read the error from errs before we read from this channel. select is explicitly designed to choose randomly.
|
I close this PR for now. I'll redesign to address https://issues.redhat.com/browse/OCPBUGS-43968 |
|
alternative fix: #2162 |
validate that there are no other parts returned by the client, and do not ignore other errors that could have happened later.