Skip to content
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

small correctness fix #609

Merged
merged 1 commit into from Jun 13, 2019

Conversation

Projects
None yet
4 participants
@BenTheElder
Copy link
Member

commented Jun 12, 2019

handle reader exiting early

we haven't observed this in our current usage, but this is more correct

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested review from amwat and neolit123 Jun 12, 2019

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

/hold

@@ -107,6 +107,7 @@ func RunWithStdoutReader(cmd Cmd, readerFunc func(io.Reader) error) error {
errChan := make(chan error, 1)
go func() {
errChan <- readerFunc(pr)

This comment has been minimized.

Copy link
@aojea

aojea Jun 12, 2019

Contributor

just learning with this, please bear with me, why do we have the defer pr.Close in L104? can we avoid having this new close by moving the defer just to L109?

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jun 12, 2019

Author Member

we want to close the reader either way, if we move line 104 here then we won't close it in the case we just fixed in #608 :-)

This comment has been minimized.

Copy link
@aojea

aojea Jun 13, 2019

Contributor

then ... it can happen that we close twice the pr pipe, right? I mean, if we close it here the previous defer will try to close it again but is already closed IIUIC

This comment has been minimized.

Copy link
@aojea

aojea Jun 13, 2019

Contributor

nevermind, the function will return an err trying to write to a broken pipe

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jun 13, 2019

Author Member

Closing twice is safe 😅

@neolit123
Copy link
Contributor

left a comment

SGTM
will leave the LGTM to @aojea

@BenTheElder

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

/hold cancel

@aojea

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 13, 2019

@k8s-ci-robot k8s-ci-robot merged commit cdf9183 into kubernetes-sigs:master Jun 13, 2019

11 of 13 checks passed

Header rules No header rules processed
Details
Pages changed 15 new files uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 3 redirect rules processed
Details
cla/linuxfoundation BenTheElder authorized
Details
deploy/netlify Deploy preview ready!
Details
pull-kind-build Job succeeded.
Details
pull-kind-conformance-parallel Job succeeded.
Details
pull-kind-conformance-parallel-1-12 Job succeeded.
Details
pull-kind-conformance-parallel-1-13 Job succeeded.
Details
pull-kind-conformance-parallel-1-14 Job succeeded.
Details
pull-kind-verify Job succeeded.
Details
tide In merge pool.
Details

@BenTheElder BenTheElder deleted the BenTheElder:correctness branch Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.