Enforce use of Gitlab::Popen instead of popen3
Problem
It's easy to accidentally introduce deadlockable code when using Open3.popen3
. For example, this script will never return due to the stderr output being longer than the pipe buffer size:
require 'open3'
long_output = '0' * 99_999
script = <<~SCRIPT
#!/bin/sh
echo "stdout"
1>&2 echo "#{long_output}"
SCRIPT
Open3.popen3(script) do |_stdin, stdout, stderr|
puts stdout.read
puts stderr.read
end
puts 'Finished.'
One of the recommended ways to avoid deadlocking is to always drain the stdout
and stderr
pipes in Thread
s so they are read from concurrently:
require 'open3'
long_output = '0' * 99_999
script = <<~SCRIPT
#!/bin/sh
echo "stdout"
1>&2 echo "#{long_output}"
SCRIPT
Open3.popen3(script) do |_stdin, stdout, stderr|
# puts stdout.read
# puts stderr.read
out_reader = Thread.new { stdout.read }
err_reader = Thread.new { stderr.read }
puts out_reader.value
puts err_reader.value
end
puts 'Finished.'
This is exactly what the methods in our Popen
module do in order to make calls to open3
safe by default.
Proposal
This proposal is to
- Introduce a new Rubocop to enforce using our existing
Popen
module in place ofOpen3.popen3
- Add developer documentation for the
Popen
module
See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/26201 for the corresponding MR for this proposal.
Proposal
-
Mention the proposal in the next backend weekly call and the #backend channel to encourage contribution -
Proceed with the proposal once 50% of the maintainers have weighed in, and 80% of the votes are 👍 -
Once approved, mention it again in the next backend weekly call and the #backend channel
Edited by Luke Duncalfe