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

[PR 1952] 2st verification - updated tests & GitHub Actions #1956

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 69 additions & 0 deletions .github/workflows/workflow.yml
@@ -0,0 +1,69 @@
name: Puma

on:
push:
branches:
- master
pull_request:
branches:
- '*'

jobs:
build:
name: >-
OS: ${{ matrix.os }} Ruby: ${{ matrix.ruby }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [ 'ubuntu-16.04', 'ubuntu-18.04', 'macos', 'windows-latest' ]
ruby: [ '2.3.x', '2.4.x', '2.5.x', '2.6.x' ]
exclude:
- os: ubuntu-16.04
ruby: 2.4.x
- os: ubuntu-16.04
ruby: 2.5.x
- os: ubuntu-16.04
ruby: 2.6.x
- os: ubuntu-18.04
ruby: 2.3.x
- os: macos
ruby: 2.3.x
- os: windows-latest
ruby: 2.3.x
steps:
- name: repo checkout
uses: actions/checkout@v1
with:
fetch-depth: 10
- name: load ruby
uses: actions/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby }}

- name: ubuntu & macos - install ragel
if: startsWith(matrix.os, 'ubuntu') || startsWith(matrix.os, 'macos')
run: |
if [ "${{ matrix.os }}" == "macos" ]; then
brew install ragel
else
sudo apt-get install ragel
fi
- name: windows - update MSYS2, openssl, ragel
if: startsWith(matrix.os, 'windows')
uses: MSP-Greg/msys2-action@master
with:
base: update
mingw: openssl ragel

- name: RubyGems, Bundler Update
run: gem update --system --no-document --conservative
- name: bundle install
run: bundle install --jobs 4 --retry 3
- name: compile
run: bundle exec rake compile
- name: test
run: bundle exec rake
env:
CI: true
TESTOPTS: -v
2 changes: 1 addition & 1 deletion .travis.yml
Expand Up @@ -45,7 +45,7 @@ matrix:
env: OS="Bionic 18.04 OpenSSL 1.1.1"
- rvm: ruby-head
env: jit=yes
- rvm: 2.4.7
- rvm: 2.4.6
os: osx
osx_image: xcode11
env: OS="osx xcode11"
Expand Down
8 changes: 0 additions & 8 deletions lib/puma/binder.rb
Expand Up @@ -50,14 +50,6 @@ def env(sock)

def close
@ios.each { |i| i.close }
@unix_paths.each do |i|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't fix the problem, as bind paths are still not removed after program stop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few days ago, I was really clear on what was where in terms of Binder @ios, @unix_paths, and @listeners. I should have added some comments...

Paths are closed in Binder#close_listeners, They aren't in Binder#close

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this assertion I've added to master. Anything that purports to fix this problem needs to pass this test (which is failing on master).

# Errno::ENOENT is intermittently raised
begin
unix_socket = UNIXSocket.new i
unix_socket.close
rescue Errno::ENOENT
end
end
end

def import_from_env
Expand Down
23 changes: 12 additions & 11 deletions lib/puma/cluster.rb
Expand Up @@ -68,7 +68,7 @@ def initialize(idx, pid, phase, options)
@pid = pid
@phase = phase
@stage = :started
@signal = "TERM"
@signal = :TERM
@options = options
@first_term_sent = nil
@started_at = Time.now
Expand All @@ -92,10 +92,6 @@ def term?
@term
end

def term?
@term
end

def ping!(status)
@last_checkin = Time.now
@last_status = status
Expand All @@ -108,7 +104,7 @@ def ping_timeout?(which)
def term
begin
if @first_term_sent && (Time.now - @first_term_sent) > @options[:worker_shutdown_timeout]
@signal = "KILL"
@signal = :KILL
else
@term ||= true
@first_term_sent ||= Time.now
Expand All @@ -119,12 +115,12 @@ def term
end

def kill
Process.kill "KILL", @pid
Process.kill :KILL, @pid
rescue Errno::ESRCH
end

def hup
Process.kill "HUP", @pid
Process.kill :HUP, @pid
rescue Errno::ESRCH
end
end
Expand Down Expand Up @@ -220,8 +216,10 @@ def check_workers(force=false)
log "- Stopping #{w.pid} for phased upgrade..."
end

w.term
log "- #{w.signal} sent to #{w.pid}..."
unless w.term?
w.term
log "- #{w.signal} sent to #{w.pid}..."
end
end
end
end
Expand Down Expand Up @@ -270,6 +268,7 @@ def worker(index, master)
server = start_server

Signal.trap "SIGTERM" do
@worker_write << "e#{Process.pid}\n" rescue nil
server.stop
end

Expand Down Expand Up @@ -501,8 +500,10 @@ def run
w.boot!
log "- Worker #{w.index} (pid: #{pid}) booted, phase: #{w.phase}"
force_check = true
when "e"
w.instance_variable_set :@term, true
when "t"
w.term
w.term unless w.term?
force_check = true
when "p"
w.ping!(result.sub(/^\d+/,'').chomp)
Expand Down
7 changes: 4 additions & 3 deletions lib/puma/launcher.rb
Expand Up @@ -217,11 +217,12 @@ def restart_args
end

def close_binder_listeners
@binder.listeners.each do |l, io|
io.close
# close binder listeners as fast as possible, so separate loop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want the listener sockets closed quickly once shutdown starts. Anything that requires checking the disk can slow down. Small issue.

@binder.listeners.each { |_, io| io.close }
@binder.listeners.each do |l, _|
uri = URI.parse(l)
next unless uri.scheme == 'unix'
File.unlink("#{uri.host}#{uri.path}")
File.unlink "#{uri.host}#{uri.path}"
end
end

Expand Down
1 change: 1 addition & 0 deletions test/config/worker_shutdown_timeout_2.rb
@@ -0,0 +1 @@
worker_shutdown_timeout 2
38 changes: 35 additions & 3 deletions test/helper.rb
Expand Up @@ -47,7 +47,11 @@ module UniquePort
@mutex = Mutex.new

def self.call
@mutex.synchronize { @port += 1 }
@mutex.synchronize {
@port += 1
@port = 3307 if @port == 3306 # MySQL on Actions
@port
}
end
end

Expand All @@ -70,6 +74,34 @@ def run(*)

module TestSkips

# finds bad signals, value is an array of symbols
BAD_SIGNAL_LIST = begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of this, what's wrong with Signal.list?

Copy link
Member Author

@MSP-Greg MSP-Greg Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows Signal.list includes :TERM, but it generates an error when used. Wasn't sure if other OS's that aren't tested might also behave the same way.

Needed a process to kill to test for it...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the idea, but let's just remove :TERM from the list if on Windows, this spawning-a-process thing feels like the nuclear option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if :TERM doesnt work on windows, why do tests that use not fail? e.g. test_term_closes_listeners or test_term_suppress

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of of course, because you can't fork on windows. D'oh!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just remove :TERM from the list if on Windows, this spawning-a-process thing feels like the nuclear option

Understandable.

I really hate defining by OS, as opposed to actually checking for functionality. Maybe it (:TERM) could change in a year or something. Probably unlikely...

code = <<-HEREDOC
data = (%w[HUP INT TTIN TTOU USR1 USR2] - Signal.list.keys).join(' ')

begin
Signal.trap('TERM') { }
rescue ArgumentError
data << 'TERM'
end

puts data
HEREDOC
pipe = IO.popen RbConfig.ruby, 'r+'
pid = pipe.pid
pipe.puts code
pipe.close_write

begin
Process.kill :TERM, pid
pipe.read.strip.split(' ').sort.map(&:to_sym)
rescue Errno::EINVAL
"#{pipe.read.strip} TERM".split(' ').sort.map(&:to_sym)
ensure
Process.wait pid
end
end

# usage: skip NO_FORK_MSG unless HAS_FORK
# windows >= 2.6 fork is not defined, < 2.6 fork raises NotImplementedError
HAS_FORK = ::Process.respond_to? :fork
Expand All @@ -82,8 +114,8 @@ module TestSkips

# usage: skip_unless_signal_exist? :USR2
def skip_unless_signal_exist?(sig, bt: caller)
signal = sig.to_s
unless Signal.list.key? signal
signal = sig.to_s.sub(/\ASIG/i, '').to_sym
if BAD_SIGNAL_LIST.include? signal
skip "Signal #{signal} isn't available on the #{RUBY_PLATFORM} platform", bt
end
end
Expand Down
8 changes: 8 additions & 0 deletions test/helpers/apps.rb
Expand Up @@ -9,4 +9,12 @@ module TestApps
[200, {"Content-Type" => "text/plain"}, ["Slept #{dly}"]]
end

# call with "GET /sleep<d> HTTP/1.1\r\n\r\n", where is the number of
# seconds to sleep
# same as rackup/sleep_pid.ru
SLEEP_PID = -> (env) do
dly = (env['REQUEST_PATH'][/\/sleep(\d+)/,1] || '0').to_i
sleep dly
[200, {"Content-Type" => "text/plain"}, ["Slept #{dly} #{Process.pid}"]]
end
end
8 changes: 8 additions & 0 deletions test/rackup/sleep_pid.ru
@@ -0,0 +1,8 @@
# call with "GET /sleep<d> HTTP/1.1\r\n\r\n", where <d> is the number of
# seconds to sleep, returns process pid

run lambda { |env|
dly = (env['REQUEST_PATH'][/\/sleep(\d+)/,1] || '0').to_i
sleep dly
[200, {"Content-Type" => "text/plain"}, ["Slept #{dly} #{Process.pid}"]]
}