unicorn:restart sends QUIT to old master before new master's workers are up

[xevix]

First, thanks for the lib, this is pretty handy. Just a quick question. I saw this pull request merged in that I think added this behavior.

#26

And in the sample unicorn file here there is also a QUIT sent to the old master:

https://github.com/sosedoff/capistrano-unicorn/blob/master/examples/rails3.rb

When I run cap unicorn:restart the task properly sends the USR2 to the old master, sleeps 2 seconds, then sends a QUIT to the old master. At this point, the workers of the new master are not yet up, so there are no workers left to handle the request, and web browsers that had started a request sit idly by. If one of the useful things of Unicorn is to have no downtime for handling requests, why is this the behavior? I'm a bit new to Unicorn as well, so please let me know if I have the wrong expectation here. Thanks.

[mskog]

I have the same issue. I tried removing the lines that quit the old process from the restart job and then it seems to work. However, I am not proficient with Unicorn either so I don't know if this is a good idea.

[xevix]

@MrCheese I did the same thing in the end in a fork. Works mostly great, but the old master takes some time to finally die I noticed (5-30 seconds). Maybe that's related. miepleinc@4944cab

[xevix]

@MrCheese Also of possible interest... #41

[DavidAllison]

@sosedoff Thanks so much for merging #41 -- this issue also probably needs some review. There are perhaps half a dozen forks that fix the problem (we're using one in production right now). The issue is that the QUIT signal really should not be sent as described in this issue's description.

We are using this branch:
miepleinc/capistrano-unicorn

Which corrects the issue.

Here's the specific commit that fixes it:
miepleinc@4944cab

So we could close #40 if we merged in that commit above.

[aspiers]

I have filed issue #45 regarding the number of forks.

[rylwin]

Agree with @DavidAllison's comment that QUIT is intended to be used differently.

Note that in the example unicorn.rb referenced in the README the QUIT signal is sent to the old master after the first child of the new master forks -- this is how the QUIT is supposed to be handled.

From the Unicorn SIGNALS documentation:
USR2 - reexecute the running binary. A separate QUIT should be sent to the original process once the child is verified to be up and running.

I agree that the commit referenced by @DavidAllison above would resolve the issue. I'd be happy to make this a pull request with the update if it would help -- just let me know.

And thanks for maintaining this repo, @sosedoff, it's been quite helpful!

[DavidAllison]

Hey guys - any update on this? Would love to see miepleinc/capistrano-unicorn merged in to @sosedoff's main repo! :)

[rylwin]

A recent commit provides a solution for this issue by adding a new task: unicorn:duplicate. This sends the USR2 signal w/o the "sleep and kill". See 8f7ffb9. If you use this instead of unicorn:restart, you get the desired result.

@sosedoff what's the reasoning for keeping the unicorn:restart task as is? It seems like this unicorn:restart` is "incorrect." What's the use case where you want to manually send QUIT to the master process after 2 seconds?

Since there is a workaround now supported should we consider this issue closed?

[DavidAllison]

@rylwin @sosedoff -- I'm happy to use unicorn:duplicate instead, but I do think the semantics are really strange. It seems like it really is the proper way to restart unicorn (and that it's an internal implementation detail that it does this by duplicating the unicorn master process while killing off workers).

[xevix]

According to Unicorn docs:
USR2 - reexecute the running binary. A separate QUIT should be sent to the original process once the child is verified to be up and running.

Where that QUIT is sent is left open to interpretation, BUT, the sample config file shows it being done this way, where QUIT is sent when the last former child of the old master dies, however commented out by default.
http://unicorn.bogomips.org/examples/unicorn.conf.rb

To support zero downtime is to me part of Unicorn's goals so this flow makes more sense to me, but so long as there's a way to make it work through another option (i.e. unicorn:duplicate) that's fine by me.

[aspiers]

So are we all agreed this can be closed now?

[sfsekaran]

@rylwin, @DavidAllison, @sosedoff, @xevix:

Proposal: Make unicorn:restart do what unicorn:duplicate currently does. Then, make unicorn:hard_restart (or whatever you want to name it) do what unicorn:restart used to do.

Reasoning: unicorn:hard_restart would be useful for hosted solutions where memory constraints prevent us from starting twice the number of workers, so it's still useful to keep around.

(I haven't taken a look at it yet, this would work if duplicate actually shuts down the old master process after the new ones have finished loading. If that's not how it works, we would simply augment it to work properly.)

[DavidAllison]

I really like that proposal. It's exactly what I would hope for in terms of the semantics of the commands.

On Mar 19, 2013, at 1:43 PM, Sathya Sekaran notifications@github.com wrote:

@rylwin, @DavidAllison, @sosedoff, @xevix:

Proposal: Make unicorn:restart do what unicorn:duplicate currently does. Then, make unicorn:hard_restart (or whatever you want to name it) do what unicorn:restart used to do.

Reasoning: unicorn:hard_restart would be useful for hosted solutions where memory constraints prevent us from starting twice the number of workers, so it's still useful to keep around.

(I haven't taken a look at it yet, this would work if duplicate actually shuts down the old master process after the new ones have finished loading. If that's not how it works, we would simply augment it to work properly.)

—
Reply to this email directly or view it on GitHub.

[rylwin]

👍 @sfsekaran

[xevix]

@sfsekaran That works.

[sfsekaran]

I've been investigating a fix for this, and I'm not entirely sure we should even stop old master manually. We should most likely be doing the standard solution (one I've implemented before) referenced here:

errbit/errbit#431

So, once you've added the proper before_fork Unicorn config block, we should provide a setting in capistrano-unicorn which disables the old master QUIT functionality entirely, so it can happen automagically when the fist Unicorn worker starts up.

The biggest benefits of this being a) cap deploy command returns much faster and b) zero downtime.

Perhaps we could provide a sample unicorn.rb file for devs to implement and a description of the provided setting to change within capistrano-unicorn afterward.

I'll be working on this in the next day or two and I'll provide a pull request for you guys to review.

[rylwin]

@sfsekaran this repo already contains a sample unicorn.rb: https://github.com/sosedoff/capistrano-unicorn/blob/master/examples/rails3.rb.

@sosedoff it would be great to get your input on this discussion

[sfsekaran]

Here's how I actually implemented it:

1. I fixed up unicorn.rb per @xevix's note (unicorn:restart sends QUIT to old master before new master's workers are up #40 (comment))
2. I switched to using unicorn:duplicate (instead of unicorn:restart)

This worked flawlessly and is on production. Next step is to fix unicorn:restart for people who still want to use it.

[sfsekaran]

FYI, I don't foresee working on a change for this soon (since the standard solution I implemented works so well). If anyone who needs it wants to write the shell code to wait for new master to be up and running before killing the old, then please don't hesitate.