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.

Sign up for GitHub

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

Jump to bottom

cluster.rb - no zombies #1887

Merged
merged 3 commits into from
Aug 6, 2019
Merged

cluster.rb - no zombies #1887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0cfb27e
cluster.rb - fixup for Cluster#check_workers, removes zombies?
MSP-Greg Aug 6, 2019
3999a44
test_integration.rb - add zombie test
MSP-Greg Aug 6, 2019
7809db9
travis.yml - misc updates
MSP-Greg Aug 6, 2019
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
18 changes: 11 additions & 7 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,32 +37,36 @@ rvm:
matrix:
fast_finish: true
include:
- rvm: 2.2
dist: trusty
- rvm: 2.2.10
Copy link
Member

Choose a reason for hiding this comment

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

In the future @MSP-Greg, I'd prefer you make changes to travis.yml in separate PRs. That way, they won't get blocked by the corresponding feature/bugfix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking the same.

While I was testing/debugging, Travis seemed to be acting up, and I wasn't sure if it was affecting the CI results I was seeing.. IOW, didn't just decide to throw it in, I was having failures...

How about I remove it right now & add another PR?

dist: trusty
env: OS="Trusty 14.04 OpenSSL 1.0.1"
- rvm: 2.6
- rvm: 2.6.3
dist: bionic
env: OS="Bionic 18.04 OpenSSL 1.1.1"
- rvm: ruby-head
env: jit=yes
- rvm: 2.4.6
os: osx
osx_image: xcode10.2
env: OS="osx xcode10.2"
osx_image: xcode11
env: OS="osx xcode11"
- rvm: 2.5.5
os: osx
osx_image: xcode10.2
env: OS="osx xcode10.2"
osx_image: xcode11
env: OS="osx xcode11"
- rvm: jruby-9.2.7.0
env: JRUBY_OPTS="--debug" JAVA_OPTS="--add-opens java.base/sun.nio.ch=org.jruby.dist --add-opens java.base/java.io=org.jruby.dist --add-opens java.base/java.util.zip=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.security.cert=ALL-UNNAMED --add-opens java.base/java.security=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED"
- rvm: jruby-head
dist: bionic
env: OS="Bionic 18.04"

allow_failures:
- rvm: ruby-head
- rvm: ruby-head
env: jit=yes
- rvm: jruby-9.2.7.0
- rvm: jruby-head
dist: bionic
env: OS="Bionic 18.04"

env:
global:
Expand Down
8 changes: 5 additions & 3 deletions lib/puma/cluster.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,11 @@ def check_workers(force=false)
# during this loop by giving the kernel time to kill them.
sleep 1 if any

@workers.reject! { |w| Process.waitpid(w.pid, Process::WNOHANG) }

@workers.reject!(&:dead?)
pids = []
while pid = Process.waitpid(-1, Process::WNOHANG) do
pids << pid
end
@workers.reject! { |w| w.dead? || pids.include?(w.pid) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Could move these lines into a reap_workers method to match cull_workers and spawn_workers.

Copy link
Member Author

@MSP-Greg MSP-Greg Aug 6, 2019 •
edited

Choose a reason for hiding this comment

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

Good suggestion, I'll leave coding style up to others. It's four lines, probably only called by the method it's contained in, and any test would involve that method...


cull_workers
spawn_workers
Expand Down
18 changes: 18 additions & 0 deletions test/test_integration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,4 +374,22 @@ def test_not_accepts_new_connections_after_term_signal
Process.wait(@server.pid)
@server = nil # prevent `#teardown` from killing already killed server
end

def test_no_zombie_children
skip NO_FORK_MSG unless HAS_FORK

worker_pids = []
server = server("-w 2 test/rackup/hello.ru")
# Get the PIDs of the child workers.
while worker_pids.size < 2
next unless line = server.gets.match(/pid: (\d+)/)
worker_pids << line.captures.first.to_i
end
# Signal the workers to terminate, and wait for them to die.
worker_pids.each { |pid| Process.kill :TERM, pid }
sleep 2

# Should return nil if Puma has correctly cleaned up
assert_nil Process.waitpid(-1, Process::WNOHANG)
end
end