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

Windows support for signal handling and dealing with IO #26

Closed
wants to merge 9 commits into from
Closed

Windows support for signal handling and dealing with IO #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
50a5bdf
fix signal and accept_nonblock to adapt to windows
naritta Sep 24, 2015
1a98304
Windows8 doens't support SIGKILL
naritta Sep 30, 2015
5bf96ee
Deleted needless change
naritta Oct 6, 2015
a87ab06
Use ServerEngine.windows? instead of platformwin
naritta Oct 6, 2015
78a8744
Add Heartbeat pipe for windows
naritta Nov 2, 2015
52f45e6
Fix including Win32
naritta Nov 10, 2015
9030144
Fix win32-pipe dependency
naritta Nov 10, 2015
f84595f
Fix Process Manager initialization to set immediate_kill_signal corre…
naritta Nov 10, 2015
45d0007
Fix pipe_name setting to generate globally unique number
naritta Nov 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions lib/serverengine/multi_spawn_server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,21 @@ module ServerEngine

class MultiSpawnServer < MultiWorkerServer
def initialize(worker_module, load_config_proc={}, &block)
@pm = ProcessManager.new(
auto_tick: false,
graceful_kill_signal: Daemon::Signals::GRACEFUL_STOP,
immediate_kill_signal: Daemon::Signals::IMMEDIATE_STOP,
enable_heartbeat: false,
)
if ServerEngine.windows?
@pm = ProcessManager.new(
auto_tick: false,
graceful_kill_signal: Daemon::Signals::GRACEFUL_STOP,
Copy link
Member

Choose a reason for hiding this comment

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

I understand that QUIT signal doesn't work on Windows. Does KILL signal work on Windows?
If not, ProcessManager#tick needs a hack like system("taskkill /f /pid #{pid}") when it sends KILL signal like:

-           Process.kill(signal, pid)
+           if ServerEngine.windows? && (signal == :KILL || signal == :SIGKILL)
+             system("taskkill /f /pid #{pid}")
+           else
+             Process.kill(signal, pid)
+           end

Copy link
Member

Choose a reason for hiding this comment

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

Or TerminateProcess win32 API.

Copy link
Member

Choose a reason for hiding this comment

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

ProcessManager#initialize sets immediate_kill_signal = true if it's not set.

immediate_kill_signal: false,
enable_heartbeat: false,
)
else
@pm = ProcessManager.new(
auto_tick: false,
graceful_kill_signal: Daemon::Signals::GRACEFUL_STOP,
immediate_kill_signal: Daemon::Signals::IMMEDIATE_STOP,
enable_heartbeat: false,
)
end

super(worker_module, load_config_proc, &block)

Expand Down
34 changes: 29 additions & 5 deletions lib/serverengine/process_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
module ServerEngine

require 'fcntl'
if ServerEngine.windows?
Copy link
Member

Choose a reason for hiding this comment

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

please add require 'serverengine/utils' here since ServerEngine.windows? is defined there.

require 'win32/pipe'
end

class ProcessManager
def initialize(config={})
Expand Down Expand Up @@ -146,9 +149,18 @@ def spawn(*args)
rpipe, wpipe = new_pipe_pair

begin
options[[wpipe.fileno]] = wpipe
if @enable_heartbeat
env['SERVERENGINE_HEARTBEAT_PIPE'] = wpipe.fileno.to_s

if ServerEngine.windows?
pipe_name = "SERVERENGINE_HEARTBEAT_PIPE_%016X" % Random.new.rand(2**128)
rpipe = Win32::Pipe::Server.new(pipe_name, 0, Win32::Pipe::ACCESS_DUPLEX | Win32::Pipe::FILE_FLAG_OVERLAPPED)
if @enable_heartbeat
env['SERVERENGINE_HEARTBEAT_PIPE'] = pipe_name
end
else
options[[wpipe.fileno]] = wpipe
if @enable_heartbeat
env['SERVERENGINE_HEARTBEAT_PIPE'] = wpipe.fileno.to_s
end
end

pid = Process.spawn(env, *args, options)
Expand Down Expand Up @@ -204,14 +216,26 @@ def tick(blocking_timeout=0)
return nil
end

ready_pipes, _, _ = IO.select(@rpipes.keys, nil, nil, blocking_timeout)
if ServerEngine.windows?
# TODO: should use WaitForMultipleObjects?
ready_pipes = @rpipes.keys
Copy link
Member

Choose a reason for hiding this comment

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

Because of semantic differences of pipes, this code could be tricky:

  • On UNIX, ProcessManager detects exist of a child process using reading from rpipe. If reading from rpipe failed with EOFError, it means that the child process exits. That's why ServerEngine can safely ignore SIGCHLD.
  • But on Windows, Win32::Pipe::Server doesn't cause EOFError even when a child process exits (if my understanding is correct). If enable_heartbeat is true, ProcessManager still can know exit of a child process. However, if it is false, ProcessManager needs to detect exit of a child process in different way.

An idea is, as you mention here in the comment, using WaitForMultipleObjects to wait for both pipe and exits of child processes.
Another idea is throw an exception at #initialize if enable_heartbeat is false on Windows.

else
ready_pipes, _, _ = IO.select(@rpipes.keys, nil, nil, blocking_timeout)
end

time ||= Time.now

if ready_pipes
ready_pipes.each do |r|
begin
r.read_nonblock(1024, @read_buffer)
if ServerEngine.windows?
read_flag = r.read
if read_flag
@read_buffer = r.buffer
end
else
r.read_nonblock(1024, @read_buffer)
end
rescue Errno::EAGAIN, Errno::EINTR
next
rescue #EOFError
Expand Down
12 changes: 7 additions & 5 deletions lib/serverengine/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,14 @@ def install_signal_handlers
s = self
SignalThread.new do |st|
st.trap(Daemon::Signals::GRACEFUL_STOP) { s.stop(true) }
st.trap(Daemon::Signals::IMMEDIATE_STOP) { s.stop(false) }
st.trap(Daemon::Signals::GRACEFUL_RESTART) { s.restart(true) }
st.trap(Daemon::Signals::IMMEDIATE_RESTART) { s.restart(false) }
st.trap(Daemon::Signals::RELOAD) { s.reload }
st.trap(Daemon::Signals::DETACH) { s.stop(true) }
st.trap(Daemon::Signals::DUMP) { Sigdump.dump }
unless ServerEngine.windows?
Copy link
Member

Choose a reason for hiding this comment

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

ok, then let's add comments here:

# Here disables signals excepting GRACEFUL_STOP == :SIGTERM because
# only SIGTERM is available on Windows.

st.trap(Daemon::Signals::IMMEDIATE_STOP) { s.stop(false) }
st.trap(Daemon::Signals::GRACEFUL_RESTART) { s.restart(true) }
st.trap(Daemon::Signals::IMMEDIATE_RESTART) { s.restart(false) }
st.trap(Daemon::Signals::RELOAD) { s.reload }
st.trap(Daemon::Signals::DUMP) { Sigdump.dump }
end
end
end

Expand Down
7 changes: 7 additions & 0 deletions lib/serverengine/utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@
#
module ServerEngine

IS_WINDOWS = /mswin|mingw/ === RUBY_PLATFORM
private_constant :IS_WINDOWS

def self.windows?
IS_WINDOWS
end

module ClassMethods
def dump_uncaught_error(e)
STDERR.write "Unexpected error #{e}\n"
Expand Down
1 change: 1 addition & 0 deletions serverengine.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Gem::Specification.new do |gem|
gem.required_ruby_version = ">= 1.9.3"

gem.add_dependency "sigdump", ["~> 0.2.2"]
gem.add_runtime_dependency("win32-pipe", ["~> 0.3.6"])

gem.add_development_dependency "rake", [">= 0.9.2"]
gem.add_development_dependency "rspec", ["~> 2.13.0"]
Expand Down