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

Avoid possible hang in util.RunCmdOut by using byte buffers instead of pipes #5220

Merged

Conversation

briandealwis
Copy link
Member

Our util.RunCmdOut() function runs commands with creating pipes for stdout and stderr, and then reads each of them to completion:

stderrPipe, err := cmd.StderrPipe()
if err != nil {
return nil, err
}
if err := cmd.Start(); err != nil {
return nil, fmt.Errorf("starting command %v: %w", cmd, err)
}
stdout, err := ioutil.ReadAll(stdoutPipe)
if err != nil {
return nil, err
}
stderr, err := ioutil.ReadAll(stderrPipe)
if err != nil {
return nil, err
}

But this code may hang if the stderr pipe fills and cause the process to block.

Since this code does not actually monitor the output streams, this PR changes the implementation to use byte buffers instead and let the Go runtime manage reading and writing.

@briandealwis briandealwis requested a review from a team as a code owner January 12, 2021 04:29
@google-cla google-cla bot added the cla: yes label Jan 12, 2021
@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #5220 (bb7e12f) into master (c62d6f5) will increase coverage by 0.04%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5220      +/-   ##
==========================================
+ Coverage   71.86%   71.90%   +0.04%     
==========================================
  Files         388      388              
  Lines       14051    14043       -8     
==========================================
  Hits        10098    10098              
+ Misses       3207     3203       -4     
+ Partials      746      742       -4     
Impacted Files Coverage Δ
pkg/skaffold/util/cmd.go 66.66% <81.81%> (+15.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c62d6f5...bb7e12f. Read the comment docs.

@briandealwis briandealwis added the kokoro:force-run forces a kokoro re-run on a PR label Jan 20, 2021
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Jan 20, 2021
@briandealwis briandealwis merged commit dafb415 into GoogleContainerTools:master Jan 20, 2021
@briandealwis briandealwis deleted the runcmdout-blocking branch January 20, 2021 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants