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

HerokuRun#run_shell! is not threadsafe #157

Closed
dzuelke opened this issue Dec 3, 2020 · 3 comments
Closed

HerokuRun#run_shell! is not threadsafe #157

dzuelke opened this issue Dec 3, 2020 · 3 comments

Comments

@dzuelke
Copy link
Contributor

dzuelke commented Dec 3, 2020

This:

    private def run_shell!
      @output = `#{@command}`
      @status = $?
    end

... will blow up with run_multi, because all sorts of things can happen in between the backticks and the reading of $?.

@schneems
Copy link
Contributor

As explained in #157, backticks followed by $? access aren't threadsafe

That's not true. $? is threadsafe:

threads = []
100.times do
  threads << Thread.new do
    value = rand(0..10)
    `exit #{value}`
    {expected: value, actual: $?.exitstatus }
  end
end

threads.join
output = threads.map(&:value)

output.each do |hash|
  raise "oh no #{hash}" if hash[:expected] != hash[:actual]
end

# => No error

or

$ irb
irb(main):001:0> $?
=> nil
irb(main):002:0>  Thread.new { `exit 1` }
=> #<Thread:0x00007fb7fe0c92f8 (irb):2 run>
irb(main):003:0> $?
=> nil
irb(main):004:0> $?
=> nil
irb(main):005:0` `exit 2`
=> ""
irb(main):006:0> $?
=> #<Process::Status: pid 71731 exit 2>

@dzuelke
Copy link
Contributor Author

dzuelke commented Jan 14, 2021

That's only within a thread, right? Outside the thread you're not guaranteed that.

So what about this:

Hatchet::Runner.new("default_ruby", run_multi: true)
  app.run_multi("ls") do |out, status|
    expect(status.success?).to be_truthy
    expect(out).to include("Gemfile")
  end
  app.run_multi("ruby -v") do |out, status|
    expect(status.success?).to be_truthy
    expect(out).to include("ruby")
  end
  puts $?
end

Which of the two runs' $? will puts print?

@dzuelke dzuelke reopened this Jan 14, 2021
@schneems
Copy link
Contributor

schneems commented Jan 14, 2021 via email

dzuelke added a commit that referenced this issue Jan 24, 2023
via option :return_obj => true

Allows accessing e.g. process status ($? is not threadsafe, and a leaky detail of the previous underlying implementation via backticks)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants