Send/recieve commands via pipe instead of sending signals #50

unak · 2016-08-10T21:44:03Z

Windows does not support interprocess signals.
Ruby emulates it a little, but not enough.
Therefore, ServerEngine should use another mechanism to send commands.
This patch implements piped command interface.
see also fluent/fluentd#1084

tagomoris · 2016-08-15T08:31:57Z

lib/serverengine/command_sender.rb

+    module Signal
+      private
+      def _stop(graceful)
+        _send_signal(!ServerEngine.windows? && graceful ? Daemon::Signals::GRACEFUL_STOP : Daemon::Signals::IMMEDIATE_STOP)


Daemon::Signals::IMMEDIATE_STOP is assigned dynamically to use KILL when it runs on Windows.
Are there any reason not to do it for GRACEFUL_STOP? Is it to show "it's not graceful on Windows" explicitly?

Because Windows cannot setd :TERM (GRACEFUL_STOP) to other process.

then why don't you redefine Signals::GRACEFUL_STOP to :KILL?

Because :KILL is not for "GRACEFUL" stop.

tagomoris · 2016-08-15T08:37:12Z

In #main in daemon.rb, there aren't so many things between code for Windows and code for others (unix-like systems).
@unak IMO it's better to split these code into two methods. How do you think about this idea?

tagomoris · 2016-08-15T08:41:50Z

lib/serverengine/multi_spawn_server.rb

+
+    def stop(stop_graceful)
+      if @command_sender == "pipe"
+        @pm.command_pipe.write stop_graceful ? "GRACEFUL_STOP\n" : "IMMEDIATE_STOP\n"


I'm not sure about the order of priority of operators and method calls.
So it's better for me to use parenthesis.

unak · 2016-08-15T08:57:51Z

@tagomoris Do you mean to write two methods windows_main and unix_main ?
The patched main is about 90 lines, and Windows specific code is only 10 lines.
I agree that main is too big, but the point of splitting should be determined by functionality, IMO.

tagomoris · 2016-08-15T09:00:36Z

spec/daemon_logger_spec.rb

-
-    log.warn "test4"*20
-    File.read("tmp/se1.log.0") =~ /test5$/
+  unless ServerEngine.windows?


Why is this test omitted for windows environment? Just for path concatenation, or any other reasons?
It's better to use pending "reason..." to show the test skipped if there are any reason to be skipped instead of using unless block.

Ah, it's because these tests are not passed on Windows (with/without this patch).
I don't know why the tests are not passed, then I can't write the reason :P
I omitted them for my purpose, but I should leave them as is.

tagomoris · 2016-08-15T09:24:13Z

@unak Right (or main_unix and main_windows :P )
That method uses two kind of pipes but one is not used in Windows environment. I think splitting these code into two methods makes much more readable than unified one.
If functionality based split is needed, we can name these methods as daemonize_with_spawn and daemonize_with_double_fork.

Extract Windows specific code and Unix specific code.

This reverts commit 779a254.

tagomoris · 2016-08-16T01:56:02Z

LGTM.
@frsyuki @naritta Could you take a glance on this?

frsyuki · 2016-08-17T03:29:00Z

lib/serverengine/daemon.rb

+
+      @pid = nil
+      @command_pipe = @config.fetch(:command_pipe, nil)
+      @command_sender = @config.fetch(:command_sender, "signal")


Signals don't work on Windows at all, is that right?
If yes, this requires all server application developers to set "pipe" if the app is running on Windows. But most of users don't want to care about the execution platform and write the same code (I believe). So it sounds better that here uses "pipe" as the default value on Windows.

Please update README.md.

Signals don't work on Windows at all, is that right?

Yes.
To sending signals to other processes, only :INT (often ignored by target processes by various reasons) and :KILL are supported on ruby on Windows.

So it sounds better that here uses "pipe" as the default value on Windows.

At first, I thought that we had to keep compatibility with already existing ServerEngine scripts.
But, rethinking now, there are no scripts which correctly be run on Windows :P
So I agree.

unak · 2016-08-17T12:48:05Z

Now this patch supports "pipe"ed Supervisor also on Unix.
Yes, I know this departs from @frsyuki's expectation, but inconsistency is removed, I hope...

tagomoris · 2016-08-18T06:17:16Z

@unak I added two comments which are also commented by @frsyuki. IMO, we can remove those value assignments from configuration simply.
Others look good to me.

tagomoris · 2016-08-19T02:22:27Z

LGTM. Thank you for making great change!

see the discussion in treasure-data#50.

unak added 12 commits August 11, 2016 06:07

Implement methods to stop, restart and so on

a61d105

Introduce CommandSender module

a7ea91d

Implement piped commands

e04f195

Use spawn instead of fork on Windows

30b0923

Use CommandSender in Supervisor

0776fd4

Use pipe command sender mechanism also in MultiSpawnServer

2f56e3c

Run daemon_spec on Windows

cea8595

Test piped command

4c5f65a

Windows has no signal which has no side effect

59fdeaf

Test Supervisor on Windows

c69e087

Log rotation test seems not to be supported on Windows

779a254

MultiProcess server does not support Windows

2c0c4ec

tagomoris reviewed Aug 15, 2016
View reviewed changes

unak added 5 commits August 16, 2016 00:15

Paren

1b46aec

Add comment

69b25f0

Ignore errors on close

d0a1240

Split main for refactoring

3206253

Extract Windows specific code and Unix specific code.

Revert "Log rotation test seems not to be supported on Windows"

1bf6197

This reverts commit 779a254.

unak mentioned this pull request Aug 15, 2016

Use windows-pr for GetModuleFileName #51

Merged

Use pending

fa63c60

tagomoris mentioned this pull request Aug 17, 2016

Adding fence for safe #49

Closed

frsyuki reviewed Aug 17, 2016
View reviewed changes

unak added 4 commits August 17, 2016 19:12

Use "pipe" as default value of command_sender on Windows

afe185e

Paren

e95d1e5

Pipe specific code should be moved into Command::Pipe

49ba784

Should support "pipe" also on Supervisor on Unix

3755005

Mention about piped command

7b485e7

Rename @command_pipe for sending because it's confusing

ffcdec8

tagomoris merged commit e791dfd into treasure-data:master Aug 19, 2016

unak deleted the win-piped-command branch August 19, 2016 02:22

unak mentioned this pull request Aug 19, 2016

Use pipe command sender mechanism of ServerEngine on Windows fluent/fluentd#1171

Merged

tagomoris mentioned this pull request Aug 22, 2016

Fix bug and unstable tests #60

Merged

This was referenced Aug 23, 2016

[WIP] Updating signal and pipe handlers used for interprocess communication #63

Closed

Removing MultiProcessServer and MultiThreadServer #64

Open

Removing heartbeat pipe from ProcessManager #65

Open

Supporting supervisor on Windows #66

Open

unak added a commit to unak/serverengine that referenced this pull request Aug 26, 2016

Remove unused assignment

fa14f72

see the discussion in treasure-data#50.

unak mentioned this pull request Aug 26, 2016

Remove unused assignment #71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send/recieve commands via pipe instead of sending signals #50

Send/recieve commands via pipe instead of sending signals #50

unak commented Aug 10, 2016

tagomoris Aug 15, 2016

unak Aug 15, 2016

frsyuki Aug 22, 2016

tagomoris Aug 22, 2016

tagomoris commented Aug 15, 2016

tagomoris Aug 15, 2016

unak commented Aug 15, 2016

tagomoris Aug 15, 2016 •

edited

Loading

unak Aug 15, 2016

tagomoris commented Aug 15, 2016

tagomoris commented Aug 16, 2016

frsyuki Aug 17, 2016

frsyuki Aug 17, 2016

unak Aug 17, 2016

unak commented Aug 17, 2016

tagomoris commented Aug 18, 2016

tagomoris commented Aug 19, 2016

Send/recieve commands via pipe instead of sending signals #50

Send/recieve commands via pipe instead of sending signals #50

Conversation

unak commented Aug 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tagomoris commented Aug 15, 2016

Choose a reason for hiding this comment

unak commented Aug 15, 2016

tagomoris Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tagomoris commented Aug 15, 2016

tagomoris commented Aug 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unak commented Aug 17, 2016

tagomoris commented Aug 18, 2016

tagomoris commented Aug 19, 2016

tagomoris Aug 15, 2016 •

edited

Loading