mongrel unicorn - Dec 2009 - "unicorn -D" always returns 0 "success" (even when failed to load)

Hi, I''m writing a Debian init script for unicorn and realized that when
starting unicorn with daemonize option, the command always returns 0, even if 
the start action failed (due for example Errno::EADDRINUSE).

Returning 0 in such case is not good as it breaks service init scripts or 
service controllers (as HeartBeat) that fully rely on the appropriate exit 
code.

Is there some way to determine if unicorn failed to start when using
"-D"?



Another related issue: When the Rack config.ru file contains some error (as a 
typo) the worker(s) returns 1 (at the moment usually). Then unicorn master 
process reapes the terminated worker process and restarts it. Of course it 
would fail again and again. Anyhow "unicorn -D" returns 0 again
(success).

Usually if a worker (all the workers) fail to start at the moment of running 
it, it obviously means that there is some error in the application with 
prevents it to run. It could be great if Unicorn could detect it.

For that I suggest something as a new option "--validation-time TIME".
Let''s
suppose TIME is 5 seconds. In case *all* the workers fail within 5 seconds 
after starting unicorn, then unicorn understands that the Rack application is 
wrong (or any other error as Errno::EADDRINUSE) so terminates all the workers 
and itself (and hopefully returns 1 or any other non-zero exit status).

Of course, all the above means that Unicorn should wait TIME seconds before 
being daemonized (so after TIME seconds it can decide which code to return).

Does it make sense? Thanks a lot.

-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> Hi, I''m writing a Debian init script for unicorn and realized that
when
> starting unicorn with daemonize option, the command always returns 0, even
if
> the start action failed (due for example Errno::EADDRINUSE).
> 
> Returning 0 in such case is not good as it breaks service init scripts or 
> service controllers (as HeartBeat) that fully rely on the appropriate exit 
> code.
> 
> Is there some way to determine if unicorn failed to start when using
"-D"?
Hi I?aki,

No way to determine that currently, as I''ve never encountered the need.

For validating startups, most folks I know have specific endpoint(s) in
application dedicated to checks.  That way they can get way more info
and all the way down into stack including things like database/memcached
connections.

Anything less is superficial because they can fail to detect other
misconfigurations (including stuff like wrong RAILS_ENV); not just
startup errors.
> Another related issue: When the Rack config.ru file contains some error (as
a
> typo) the worker(s) returns 1 (at the moment usually). Then unicorn master 
> process reapes the terminated worker process and restarts it. Of course it 
> would fail again and again. Anyhow "unicorn -D" returns 0 again
(success).
> 
> Usually if a worker (all the workers) fail to start at the moment of
running
> it, it obviously means that there is some error in the application with 
> prevents it to run. It could be great if Unicorn could detect it.
I''m generally hesitant to introduce code/features/bloat that
aren''t
helpful to people who properly test configurations before deploying :)

Adding this code doesn''t solve problems.  Either way the site can
become
inaccessible/unusable and problems are noticeable very quickly.

If there''s sufficient interest, I''ll consider accepting a
small patch
for this.  Right now I''m unconvinced...
> For that I suggest something as a new option "--validation-time
TIME". Let''s
> suppose TIME is 5 seconds. In case *all* the workers fail within 5 seconds 
> after starting unicorn, then unicorn understands that the Rack application
is
> wrong (or any other error as Errno::EADDRINUSE) so terminates all the
workers
> and itself (and hopefully returns 1 or any other non-zero exit status).
> 
> Of course, all the above means that Unicorn should wait TIME seconds before
> being daemonized (so after TIME seconds it can decide which code to
return).
> 
> Does it make sense? Thanks a lot.
We avoid introducing command-line options since they''re unlikely to be
compatible with "rackup" parsing of the "#\" lines in .ru
files.

-- 
Eric Wong

El Mi?rcoles, 23 de Diciembre de 2009, Eric Wong escribi?:
> No way to determine that currently, as I''ve never encountered the
need.
> 
> For validating startups, most folks I know have specific endpoint(s) in
> application dedicated to checks.  That way they can get way more info
> and all the way down into stack including things like database/memcached
> connections.
Yes, you are right and I agree now. I was thinking in Unicorn as a final 
application rather than a HTTP applications container. It''s like
Apache, no
body expects Apache to return 1 and exit in case a PHP application has a typo 
:)


> > Usually if a worker (all the workers) fail to start at the moment of
> > running it, it obviously means that there is some error in the
> > application with prevents it to run. It could be great if Unicorn
could
> > detect it.
> 
> I''m generally hesitant to introduce code/features/bloat that
aren''t
> helpful to people who properly test configurations before deploying :)
> 
> Adding this code doesn''t solve problems.  Either way the site can
become
> inaccessible/unusable and problems are noticeable very quickly.
Sure, but how to explain that to Hearteat? :)

 
> If there''s sufficient interest, I''ll consider accepting a
small patch
> for this.  Right now I''m unconvinced...
I''ve taken a look to the code and such a feature would definitively
make it
ugly (IMHO).

So I''m thinking in a different approach: a custom script to check the
status
of the application. Such script would communicate with the application (maybe 
using DRB). If the DRB connection fails (because the workers fail to start) 
then it means that something wrong occurs. And such script would also return 
the whole status (DB connections and so).
I would include such script as "status" option in a service init
script. The
"start" action would call "status" after N seconds and
decide if the
*application* is running or not (if not then kill unicorn and exit 1).

PS: Since Unicorn makes usage of signals, is there any way to determine (by 
using a signal) if the process running with some PID is an unicorn process?

This is: usually to verify the process status the PID file/value is inspected. 
If there is a process running under such PID then we "assume" that
process is
Unicorn. In case that PID is stolen by any other process (since PID file 
wasn''t deleted) nobody realizes of it.

A way to determine it could be asking to Unicorn master process (using i.e. 
DRB) its PID and match it against the PID value stored in the pidfile. Of 
course it would require some kind of communication with unicorn master process 
to get its PID, is it possible now in some way?



 
> > For that I suggest something as a new option "--validation-time
TIME".
> > Let''s suppose TIME is 5 seconds. In case *all* the workers
fail within 5
> > seconds after starting unicorn, then unicorn understands that the Rack
> > application is wrong (or any other error as Errno::EADDRINUSE) so
> > terminates all the workers and itself (and hopefully returns 1 or any
> > other non-zero exit status).
> >
> > Of course, all the above means that Unicorn should wait TIME seconds
> > before being daemonized (so after TIME seconds it can decide which
code
> > to return).
> >
> > Does it make sense? Thanks a lot.
> 
> We avoid introducing command-line options since they''re unlikely
to be
> compatible with "rackup" parsing of the "#\" lines in
.ru files.
I didn''t know about such options in .ru file. Are those options
supposed to be
read by the backend?


Thanks a lot.

-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El Mi?rcoles, 23 de Diciembre de 2009, Eric Wong escribi?:
> > > Usually if a worker (all the workers) fail to start at the moment
of
> > > running it, it obviously means that there is some error in the
> > > application with prevents it to run. It could be great if Unicorn
could
> > > detect it.
> > 
> > I''m generally hesitant to introduce code/features/bloat that
aren''t
> > helpful to people who properly test configurations before deploying :)
> > 
> > Adding this code doesn''t solve problems.  Either way the site
can become
> > inaccessible/unusable and problems are noticeable very quickly.
> 
> Sure, but how to explain that to Hearteat? :)
I had a lot of trouble coming to terms with this one myself :)

Eventually I reasoned that the heartbeat mechanism is to protect against
unforeseen failures way down the line (and sometimes outside of the
application''s control).

It''s (hopefully) common and expected to check applications shortly
following deployments, so heartbeat isn''t for that.

However applications have a tendency to break in unforseen ways, and
only in production, and at the worst times.  These failures happen well
after the app is deployed :)

In my experience, it''s often the least used/trafficked parts of the
application that fail, too.

Of course if you write perfect software that handles every possible
failure case and don''t need heartbeat (nor multiple processes because
your code is super-efficient :), check out Zbatery[1]

[1] http://zbatery.bogomip.org/
> > If there''s sufficient interest, I''ll consider
accepting a small patch
> > for this.  Right now I''m unconvinced...
> 
> I''ve taken a look to the code and such a feature would
definitively make it
> ugly (IMHO).
> 
> So I''m thinking in a different approach: a custom script to check
the status
> of the application. Such script would communicate with the application
(maybe
> using DRB). If the DRB connection fails (because the workers fail to start)
> then it means that something wrong occurs. And such script would also
return
> the whole status (DB connections and so).
> I would include such script as "status" option in a service init
script. The
> "start" action would call "status" after N seconds and
decide if the
> *application* is running or not (if not then kill unicorn and exit 1).
> 
> PS: Since Unicorn makes usage of signals, is there any way to determine (by
> using a signal) if the process running with some PID is an unicorn process?
> 
> This is: usually to verify the process status the PID file/value is
inspected.
> If there is a process running under such PID then we "assume"
that process is
> Unicorn. In case that PID is stolen by any other process (since PID file 
> wasn''t deleted) nobody realizes of it.
Under modern Linux systems, you have several options...

  tr ''\0'' '' '' < /proc/$PID/cmdline # show
the command-line

  pmap $PID | grep unicorn_http.so # not many other things load this

  lsof -p $PID  # can check listening ports/log files

Unfortunately I don''t use other OSes enough to remember the
equivalents...
> A way to determine it could be asking to Unicorn master process (using i.e.
> DRB) its PID and match it against the PID value stored in the pidfile. Of 
> course it would require some kind of communication with unicorn master
process
> to get its PID, is it possible now in some way?
I don''t see how being under a specific PID actually matters.  Why not
just write a script that sends an HTTP request to check?

Workers will die soon after the first request, or (timeout/2) seconds if
a master dies anyways.
> > We avoid introducing command-line options since they''re
unlikely to be
> > compatible with "rackup" parsing of the "#\" lines
in .ru files.
> 
> I didn''t know about such options in .ru file. Are those options
> supposed to be read by the backend?
I don''t believe it''s well-documented, I only found out from
reading the
rackup code and I''m not really a fan of magic comments, but
they''re only
for the basic command-line arguments rackup handles:

  $ rackup -h
  Usage: rackup [ruby options] [rack options] [rackup config]
  Ruby options:
    -e, --eval LINE          evaluate a LINE of code
    -d, --debug              set debugging flags (set $DEBUG to true)
    -w, --warn               turn warnings on for your script
    -I, --include PATH       specify $LOAD_PATH (may be used more than once)
    -r, --require LIBRARY    require the library, before executing your script
  Rack options:
    -s, --server SERVER      serve using SERVER (webrick/mongrel)
    -o, --host HOST          listen on HOST (default: 0.0.0.0)
    -p, --port PORT          use PORT (default: 9292)
    -E, --env ENVIRONMENT    use ENVIRONMENT for defaults (default: development)
    -D, --daemonize          run daemonized in the background
    -P, --pid FILE           file to store PID (default: rack.pid)
  Common options:
    -h, --help               Show this message
        --version            Show version

-- 
Eric Wong

El Mi?rcoles, 23 de Diciembre de 2009, Eric Wong
escribi?:
> > > Adding this code doesn''t solve problems.  Either way the
site can
> > > become inaccessible/unusable and problems are noticeable very
quickly.
> >
> > Sure, but how to explain that to Hearteat? :)
> 
> I had a lot of trouble coming to terms with this one myself :)
> 
> Eventually I reasoned that the heartbeat mechanism is to protect against
> unforeseen failures way down the line (and sometimes outside of the
> application''s control).
> 
> It''s (hopefully) common and expected to check applications shortly
> following deployments, so heartbeat isn''t for that.
Yes, you are right.

 
> However applications have a tendency to break in unforseen ways, and
> only in production, and at the worst times.  These failures happen well
> after the app is deployed :)
Yes, every application breaks just in the moment that it shouldn''t :)

However I''m building my app now and when start it it usually has
errors, and
that''s good as it means I''m working :)
But sincerelly I would like to see the error just once rather than many times 
due to workers respawn :)

Also, is there any "interval" parameter so a worker is not re-spawned
until
such timeout (after die).



> > This is: usually to verify the process status the PID file/value is
> > inspected. If there is a process running under such PID then we
"assume"
> > that process is Unicorn. In case that PID is stolen by any other
process
> > (since PID file wasn''t deleted) nobody realizes of it.
> 
> Under modern Linux systems, you have several options...
> 
>   tr ''\0'' '' '' < /proc/$PID/cmdline #
show the command-line
> 
>   pmap $PID | grep unicorn_http.so # not many other things load this
> 
>   lsof -p $PID  # can check listening ports/log files
Yes! this is much better! :)

 
> Unfortunately I don''t use other OSes enough to remember the
> equivalents...
The server will run just on Linux (Unix) server :)

> > A way to determine it could be asking to Unicorn master process (using
> > i.e. DRB) its PID and match it against the PID value stored in the
> > pidfile. Of course it would require some kind of communication with
> > unicorn master process to get its PID, is it possible now in some way?
> 
> I don''t see how being under a specific PID actually matters.  Why
not
> just write a script that sends an HTTP request to check?
> 
> Workers will die soon after the first request, or (timeout/2) seconds if
> a master dies anyways.
Well, in fact I want to code some console utility to get information from the 
application (loaded configuration, check the database(s) connection and so).
For that I was thinking in a DRb server running in the master because it must 
bind in some TCP port.

The problem is that I should stop the DRb server before master reexec to avoid 
EADDINUSE. Well, I could code some rescue block...



> > I didn''t know about such options in .ru file. Are those
options
> > supposed to be read by the backend?
> 
> I don''t believe it''s well-documented, I only found out
from reading the
> rackup code and I''m not really a fan of magic comments, but
they''re only
> for the basic command-line arguments rackup handles:
> 
>   $ rackup -h
...
>  -D, --daemonize          run daemonized in the background
Why "daemonize" is not present in unicornf configuration file?


Really thanks a lot for all your help.



-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El Mi?rcoles, 23 de Diciembre de 2009, Eric Wong escribi?:
> However I''m building my app now and when start it it usually has
errors, and
> that''s good as it means I''m working :)
> But sincerelly I would like to see the error just once rather than many
times
> due to workers respawn :)
> 
> Also, is there any "interval" parameter so a worker is not
re-spawned until
> such timeout (after die).
You can sleep in the before_fork hook.
> Well, in fact I want to code some console utility to get information from
the
> application (loaded configuration, check the database(s) connection and
so).
> For that I was thinking in a DRb server running in the master because it
must
> bind in some TCP port.
 
DRb can bind to UNIX domain sockets, too.  But ask yourself:

  Is your app really so broken that you always need DRb running on it?

You may also want to checkout Hijack, I haven''t used/needed it myself,
but it could be interesting if an app is really in a lot of trouble:

http://www.rubyinside.com/hijack-get-a-live-irb-prompt-for-any-existing-ruby-process-2232.html
> The problem is that I should stop the DRb server before master reexec to
avoid
> EADDINUSE. Well, I could code some rescue block...
rescuing EADDRINUSE in a loop is also how the "listen" directive
implements the :tries/:delay parameters.
> >   $ rackup -h
> ...
> >  -D, --daemonize          run daemonized in the background
> 
> Why "daemonize" is not present in unicornf configuration file?
Mainly it was cleaner and easier to test/implement it the way it is now.
It''s also easier for somebody testing a setup to toggle it from the
command line.

I actually didn''t want to support --daemonize at all since setsid(8),
is standard most GNU/Linux distros nowadays, but we support more than
just GNU/Linux.
> Really thanks a lot for all your help.
No problem.

-- 
Eric Wong

El Jueves, 24 de Diciembre de 2009, Eric Wong escribi?:
> I?aki Baz Castillo <ibc at aliax.net> wrote:
> > El Mi?rcoles, 23 de Diciembre de 2009, Eric Wong escribi?:
> > However I''m building my app now and when start it it usually
has errors,
> > and that''s good as it means I''m working :)
> > But sincerelly I would like to see the error just once rather than
many
> > times due to workers respawn :)
> >
> > Also, is there any "interval" parameter so a worker is not
re-spawned
> > until such timeout (after die).
> 
> You can sleep in the before_fork hook.
You mean "before_exec", rigth? :)

Yes, it''s the solution :)



> I actually didn''t want to support --daemonize at all since
setsid(8),
> is standard most GNU/Linux distros nowadays, but we support more than
> just GNU/Linux.
AFAIK setsid is not 100% valid to daemonize a process because:

- It doesn''t handle the exit return code of the process execution (it
just
returns 0 even if the process returns 1 at startup).

- It remains writting in the current stdout (the terminal in which you run 
"setsid program_name".


Thanks a lot.


-- 
I?aki Baz Castillo <ibc at aliax.net>

El Mi?rcoles, 23 de Diciembre de 2009, Eric Wong
escribi?:
> > Usually if a worker (all the workers) fail to start at the moment of
> > running  it, it obviously means that there is some error in the
> > application with prevents it to run. It could be great if Unicorn
could
> > detect it.
> 
> I''m generally hesitant to introduce code/features/bloat that
aren''t
> helpful to people who properly test configurations before deploying :)
> 
> Adding this code doesn''t solve problems.  Either way the site can
become
> inaccessible/unusable and problems are noticeable very quickly.
Ok, I should explain better why I need this feature:

I''m not coding a HTTP app on top of Unicorn to host in a server.
Instead I''m
building a XCAP server (RFC 4825) using Rack and Unicorn. I''ll also
publish a
Ruby gem containing all the server (Unicorn + Rack application).

So, any user could install the gem, set the database(s), fill the 
configuration file and star the server in background by running the provided 
init script ("/etc/init.d/xcapserver start") which runs the server
daemonized.

Imagine that the user wrote a typo, i.e:

  after_fork do |server, worker|
    # Note the space typo:
    worker.u ser("www-data", "www-data") if Process.euid ==
0
  end

Then the init script would return 0 (success) but the server wouldn''t
work.
The user would check "ps aux | grep xcapserver" and would see the
xcapserver
running. It would be complex to understand why the server it''s failing.

So it would be great the following:

- The user start the init script (which calls "unicorn -D ...") with a
hypotethical new option ("--no-error-time 2").

- The command "unicorn -D" remains in foreground for 2 seconds before
master
process going to background.

- Unicorn fails to create the workers at the moment (so within 2 seconds).

- Instead of re-spawning them, the command (still in foreground) exits with 
return code 1 (error).




But there is other case which is much wrose IMHO. Imagine the user writes a 
space typo:

    stde rr_path "/var/log/xcapserver.err.log"

or imagine it uses a path that doesn''t exist (/var/log/xcapserver/
doesn''t
exist):

    stderr_path "/var/log/xcapserver/err.log"

In both cases "unicorn -D" returns 0 but the server is not running (no
unicorn
process running). So, why did it return 0? It''s not an error when
creating the
workers, but when running also the master process. IMHO in this case Unicorn 
should return non zero (even if it called with "-D").


I''m playing right now with unicorn/launcher.rb (daemonize! method)but I
get
nothing. Unfortunatelly I think that the usage of fork makes very difficult 
the main command to behave as I desire (exit status).

If you could give me some tips I''d try to implement it.

Reall thanks a lot again and again :)




-- 
I?aki Baz Castillo <ibc at aliax.net>

El Viernes, 25 de Diciembre de 2009, I?aki Baz Castillo
escribi?:
> AFAIK setsid is not 100% valid to daemonize a process because:
> 
> - It doesn''t handle the exit return code of the process execution
(it just
> returns 0 even if the process returns 1 at startup).
> 
> - It remains writting in the current stdout (the terminal in which you run 
> "setsid program_name".
Forget my last comment as the redirection must be done for the program to run. 


-- 
I?aki Baz Castillo <ibc at aliax.net>

El S?bado, 26 de Diciembre de 2009, I?aki Baz Castillo
escribi?:
> I''m playing right now with unicorn/launcher.rb (daemonize!
method)but I
>  get  nothing. Unfortunatelly I think that the usage of fork makes very
>  difficult the main command to behave as I desire (exit status).
> 
> If you could give me some tips I''d try to implement it.
I''ve implemented it! :)

If you are interested please review this file (I cannot attach it in the mail 
as it''s rejected by the maillist):

  http://oversip.net/public/min_time_running.rb

It contains a modification for bin/unicorn and a rewritten 
lib/unicorn/launcher.rb.

The code works for me for the two cases I explained in my previous mail.
Of course it''s an optional feature (I''ve implemented it with
"-M --min-
running-time SECONDS" in the options parser).

Regards.

-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El S?bado, 26 de Diciembre de 2009, I?aki Baz Castillo escribi?:
> > I''m playing right now with unicorn/launcher.rb (daemonize!
method)but I
> >  get  nothing. Unfortunatelly I think that the usage of fork makes
very
> >  difficult the main command to behave as I desire (exit status).
> > 
> > If you could give me some tips I''d try to implement it.
> 
> I''ve implemented it! :)
> 
> If you are interested please review this file (I cannot attach it in the
mail
> as it''s rejected by the maillist):
> 
>   http://oversip.net/public/min_time_running.rb
> 
> It contains a modification for bin/unicorn and a rewritten 
> lib/unicorn/launcher.rb.
Interesting, I could be tempted to accept this with a few changes...

Process.detach() spawns a background Thread.  Having running threads
before forking won''t fly with certain OS threading libraries (some
versions of FreeBSD, I believe, check MRI Redmine for some open bugs).

What you''re trying to accomplish does not require threads.

Use trap(Symbol), trap(Integer) is harder to read and has a likelyhood
of being non-portable for certain signals.  Likewise with Process.kill
(0 being the only exception, of course).

I would trap() in the parent before any forking, in case the
sleep time is ever so low there''s a race condition.  Probably no
need to handle USR1, either, just let the parent die on its own,
or alternatively, have the parent sleep forever in case something
unforseen happens in the children...
> The code works for me for the two cases I explained in my previous mail.
> Of course it''s an optional feature (I''ve implemented it
with "-M --min-
> running-time SECONDS" in the options parser).
I believe the nginx grace period is hard-coded at 5 seconds, which should
be enough.

-- 
Eric Wong

El S?bado, 26 de Diciembre de 2009, Eric Wong escribi?:
> >   http://oversip.net/public/min_time_running.rb
> >
> > It contains a modification for bin/unicorn and a rewritten
> > lib/unicorn/launcher.rb.
> 
> Interesting, I could be tempted to accept this with a few changes...
> 
> Process.detach() spawns a background Thread.  Having running threads
> before forking won''t fly with certain OS threading libraries (some
> versions of FreeBSD, I believe, check MRI Redmine for some open bugs).
Yes, but there is a problem:
Imagine I don''t use Process.detach and the master process ends when
loads the
rack application (due to a typo or due to an error in unicorn conf file).
Then the master process would remain visible/alive but as zombie.
If so, when the controller process sends signal 0 to check the master process 
it will always receive ''true'' (a zombie process is a process
which can receive
signals, it''s does exist).

If I could check if the master process is zombie then I wouldn''t need 
Process.detach, but I don''t know how to check if a process is zombie,
no way
in:
  http://ruby-doc.org/core/classes/Process.html
  http://ruby-doc.org/core/classes/Process/Status.html

Any suggestion here?

 
> Use trap(Symbol), trap(Integer) is harder to read and has a likelyhood
> of being non-portable for certain signals.  Likewise with Process.kill
> (0 being the only exception, of course).
Sure, I''ll change it.

 
> I would trap() in the parent before any forking, in case the
> sleep time is ever so low there''s a race condition.
Ok.

> Probably no need to handle USR1, either, just let the parent die on its
own,
Note that the parent sleeps for ''min_running_time'' + 1 (to
avoid exiting
before receiving USR1 or USR2 from the controller process). But if the 
controller process sends USR1 then the parent can already exit without waiting 
one second more.
Well, this is not very interesting, but I think it''s good in order to
get a
faster exit code (it''s not a good idea that a daemon takes long time to
start
and return ans exit status).

> or alternatively, have the parent sleep forever in case something
> unforseen happens in the children...
But then the parent process doesn''t end and continues in foreground.
This is
not valid for running as daemon. The parent process must exit with the 
appropriate status code after a few seconds to behave as any service running 
in background.

> > The code works for me for the two cases I explained in my previous
mail.
> > Of course it''s an optional feature (I''ve implemented
it with "-M --min-
> > running-time SECONDS" in the options parser).
> 
> I believe the nginx grace period is hard-coded at 5 seconds, which should
> be enough.
Sorry, I don''t know ngix. I need to use other reverse proxy (Pound) but
let me
open a new thread about it :)

Anyhow I don''t understand what you mean with "nginx grace
period". How does
affect to Unicorn? Do you mean that, in case of implementing the above, 
Unicorn wouldn''t need a --min-running-time option? (I expect it would
be
required since Unicorn is 100% independent of ngix)



Really thanks a lot. I''ll do the changes you suggest and show you again
(but
still don''t know how to avoid Process.detach for now...).



-- 
I?aki Baz Castillo <ibc at aliax.net>

El S?bado, 26 de Diciembre de 2009, I?aki Baz Castillo
escribi?:
> El S?bado, 26 de Diciembre de 2009, Eric Wong escribi?:
> > >   http://oversip.net/public/min_time_running.rb
> > >
> > > It contains a modification for bin/unicorn and a rewritten
> > > lib/unicorn/launcher.rb.
> >
> > Interesting, I could be tempted to accept this with a few changes...
> >
> > Process.detach() spawns a background Thread.  Having running threads
> > before forking won''t fly with certain OS threading libraries
(some
> > versions of FreeBSD, I believe, check MRI Redmine for some open bugs).
> 
> Yes, but there is a problem:
> Imagine I don''t use Process.detach and the master process ends
when loads
>  the rack application (due to a typo or due to an error in unicorn conf
>  file). Then the master process would remain visible/alive but as zombie.
> If so, when the controller process sends signal 0 to check the master
>  process it will always receive ''true'' (a zombie process
is a process which
>  can receive signals, it''s does exist).

Hi, I''ve improved it a bit with your suggestions and also simplified it
a lot.
Now there is no a "controller process". Instead, if
"Unicorn.run" raises then
master process rescues the exception and sends USR1 to parent process so it 
exists with 1.
So, Process.detach is not used anymore :)

Please check it from here:

  http://oversip.net/public/unicorn_addons.rb


> > Use trap(Symbol), trap(Integer) is harder to read and has a likelyhood
> > of being non-portable for certain signals.  Likewise with Process.kill
> > (0 being the only exception, of course).
Done in new code.

 
> > I would trap() in the parent before any forking, in case the
> > sleep time is ever so low there''s a race condition.
Done in new code.

 
> > Probably no need to handle USR1, either, just let the parent die on
its
> > own,
Now just one signal plays: USR1. It is sent by master process to parent 
process if Unicorn.run raised an exception.


Regards.


-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El S?bado, 26 de Diciembre de 2009, I?aki Baz Castillo escribi?:
> > El S?bado, 26 de Diciembre de 2009, Eric Wong escribi?:
> > > Interesting, I could be tempted to accept this with a few
changes...
> > >
> > > Process.detach() spawns a background Thread.  Having running
threads
> > > before forking won''t fly with certain OS threading
libraries (some
> > > versions of FreeBSD, I believe, check MRI Redmine for some open
bugs).
> > 
> > Yes, but there is a problem:
> > Imagine I don''t use Process.detach and the master process
ends when loads
> >  the rack application (due to a typo or due to an error in unicorn
conf
> >  file). Then the master process would remain visible/alive but as
zombie.
> > If so, when the controller process sends signal 0 to check the master
> >  process it will always receive ''true'' (a zombie
process is a process which
> >  can receive signals, it''s does exist).
> 
> Hi, I''ve improved it a bit with your suggestions and also
simplified
> it a lot.  Now there is no a "controller process". Instead, if
> "Unicorn.run" raises then master process rescues the exception
and
> sends USR1 to parent process so it exists with 1.
Hi, I realized Unicorn.run inside the daemonize method doesn''t work.
The daemonize method is also used by Rainbows! (and Zbatery).

However, I''ve reimplemented much of your idea here so it does not
involve sleeping at all, but rather shares a pipe with the grandparent
(started by the user) and Unicorn master process.  The added benefit of
using a pipe is that there''s no fuzzy sleeping involved at or grace
periods to worry about, too.

Also pushed out to the "ready_pipe" branch of
git://git.bogomips.org/unicorn.git

Let me know how it goes.

If everything looks good, I''ll consider merging for 0.96.0.
>From 5eea32764571b721cd1a89cf9ebfa853c621ac9e Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson at yhbt.net>
Date: Sat, 26 Dec 2009 17:04:57 -0800
Subject: [PATCH] exit with failure if master dies when daemonized
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This behavior change also means our grandparent (launched
from a controlling terminal or script) will wait until
the master process is ready before returning.

Thanks to I?aki Baz Castillo for the initial implementations
and inspiration.
---
 bin/unicorn             |    2 +-
 bin/unicorn_rails       |    2 +-
 lib/unicorn.rb          |    9 +++++++--
 lib/unicorn/launcher.rb |   37 +++++++++++++++++++++++++++++--------
 test/exec/test_exec.rb  |    2 +-
 5 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/bin/unicorn b/bin/unicorn
index 651c2ff..5af021d 100755
--- a/bin/unicorn
+++ b/bin/unicorn
@@ -161,5 +161,5 @@ if $DEBUG
   })
 end
 
-Unicorn::Launcher.daemonize! if daemonize
+Unicorn::Launcher.daemonize!(options) if daemonize
 Unicorn.run(app, options)
diff --git a/bin/unicorn_rails b/bin/unicorn_rails
index 4a22a8c..b1458fc 100755
--- a/bin/unicorn_rails
+++ b/bin/unicorn_rails
@@ -202,6 +202,6 @@ end
 
 if daemonize
   options[:pid] = rails_pid
-  Unicorn::Launcher.daemonize!
+  Unicorn::Launcher.daemonize!(options)
 end
 Unicorn.run(app, options)
diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index 71d5994..ae05f03 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -25,7 +25,8 @@ module Unicorn
 
   class << self
     def run(app, options = {})
-      HttpServer.new(app, options).start.join
+      ready_pipe = options.delete(:ready_pipe)
+      HttpServer.new(app, options).start.join(ready_pipe)
     end
   end
 
@@ -313,7 +314,7 @@ module Unicorn
     # (or until a termination signal is sent).  This handles signals
     # one-at-a-time time and we''ll happily drop signals in case
somebody
     # is signalling us too often.
-    def join
+    def join(ready_pipe = nil)
       # this pipe is used to wake us up from select(2) in #join when signals
       # are trapped.  See trap_deferred
       init_self_pipe!
@@ -324,6 +325,10 @@ module Unicorn
       trap(:CHLD) { |sig_nr| awaken_master }
       proc_name ''master''
       logger.info "master process ready" # test_exec.rb relies on
this message
+      if ready_pipe
+        ready_pipe.syswrite($$.to_s)
+        ready_pipe.close
+      end
       begin
         loop do
           reap_all_workers
diff --git a/lib/unicorn/launcher.rb b/lib/unicorn/launcher.rb
index 1229b84..2d6ad97 100644
--- a/lib/unicorn/launcher.rb
+++ b/lib/unicorn/launcher.rb
@@ -19,21 +19,42 @@ class Unicorn::Launcher
   #     the directory it was started in when being re-executed
   #     to pickup code changes if the original deployment directory
   #     is a symlink or otherwise got replaced.
-  def self.daemonize!
+  def self.daemonize!(options = {})
     $stdin.reopen("/dev/null")
+    $stdin.sync = true # may not do anything...
 
     # We only start a new process group if we''re not being reexecuted
     # and inheriting file descriptors from our parent
     unless ENV[''UNICORN_FD'']
-      exit if fork
-      Process.setsid
-      exit if fork
+      # grandparent - reads pipe, exits when master is ready
+      #  \_ parent  - exits immediately ASAP
+      #      \_ unicorn master - writes to pipe when ready
 
-      # $stderr/$stderr can/will be redirected separately in the Unicorn config
-      Unicorn::Configurator::DEFAULTS[:stderr_path] = "/dev/null"
-      Unicorn::Configurator::DEFAULTS[:stdout_path] = "/dev/null"
+      rd, wr = IO.pipe
+      grandparent = $$
+      if fork
+        wr.close # grandparent does not write
+      else
+        rd.close # unicorn master does not read
+        Process.setsid
+        exit if fork # parent dies now
+      end
+
+      if grandparent == $$
+        # this will block until HttpServer#join runs (or it dies)
+        master_pid = rd.sysread(16).to_i
+        exit!(1) unless master_pid > 1
+        exit 0
+      else # unicorn master process
+        options[:ready_pipe] = wr
+        # $stderr/$stderr can/will be redirected separately in the
+        # Unicorn config
+        Unicorn::Configurator::DEFAULTS[:stderr_path] = "/dev/null"
+        Unicorn::Configurator::DEFAULTS[:stdout_path] = "/dev/null"
+
+        # returns so Unicorn.run can happen
+      end
     end
-    $stdin.sync = $stdout.sync = $stderr.sync = true
   end
 
 end
diff --git a/test/exec/test_exec.rb b/test/exec/test_exec.rb
index fc0719b..24ba856 100644
--- a/test/exec/test_exec.rb
+++ b/test/exec/test_exec.rb
@@ -805,7 +805,7 @@ EOF
       exec($unicorn_bin, "-D", "-l#{@addr}:#{@port}",
"-c#{ucfg.path}")
     end
     pid, status = Process.waitpid2(pid)
-    assert status.success?, "original process exited successfully"
+    assert ! status.success?, "original process exited successfully"
     sleep 1 # can''t waitpid on a daemonized process :<
     assert err.stat.size > 0
   end
-- 
Eric Wong

El Domingo, 27 de Diciembre de 2009, Eric Wong escribi?:
> > Hi, I''ve improved it a bit with your suggestions and also
simplified
> > it a lot.  Now there is no a "controller process". Instead,
if
> > "Unicorn.run" raises then master process rescues the
exception and
> > sends USR1 to parent process so it exists with 1.
> 
> Hi, I realized Unicorn.run inside the daemonize method doesn''t
work.
Strange, it works for me...

 
> However, I''ve reimplemented much of your idea here so it does not
> involve sleeping at all, but rather shares a pipe with the grandparent
> (started by the user) and Unicorn master process.  The added benefit of
> using a pipe is that there''s no fuzzy sleeping involved at or
grace
> periods to worry about, too.
> 
> Also pushed out to the "ready_pipe" branch of
> git://git.bogomips.org/unicorn.git
> 
> Let me know how it goes.
> 
> If everything looks good, I''ll consider merging for 0.96.0.
It''s really awesome! I''ve tested it and it works great, and
avoids the
sleeping stuff and a new commandlline option. Congratulations :)

However there is still a not solved case. Let me please explain it with two 
examples:

1) In "after_fork" I set:
     worker.user("non_existing_user","non_existing_group")

The master process would start properly and would notify "success" to 
grandparent. (so the init script returns 0). But the fact is that all the 
workers fail to start and are respawned again and again.


2) I use "Clogger" in my Rack config.ru. But I set a log file in a
path that
doesn''t exist. Clogger raises due to this (it doesn''t try to
create the full
path).
Again the master process has notified "success" to grandparent (exis
status 0
then) but the workers are respawned again and again.


In both cases, if "preload_app" is false then doing a "ps" I
see new master
processes being created by first master process (with new workers) and all of 
them die while new ones born.
If "preload_app" is false then there are no borning master processes
as just
the workers fail to start when loading the Rack app.


It would be great if the same mechanims (master notifies to grandparent) would 
be implemented between first process loading the Rack app (a worker if 
"preload_app") and master process, so if the first worker fails when
loading
the Rack app then master process doesn''t notify "success" to
grandparent and
exists (instead of respawning again and again the workers).

If "preload_app" is true then I don''t understand why your new
code doesn''t
work right now. I expect that master process notifies "success" to
grandparent
before loading the Rack app, am I right? would make sense master process to 
wait until Rack app is loaded and if it fails then notify "error" and
exit
completely?


Kind regards and thanks a lot for your great work.











-- 
I?aki Baz Castillo <ibc at aliax.net>

El Domingo, 27 de Diciembre de 2009, I?aki Baz Castillo
escribi?:
> In both cases, if "preload_app" is false then doing a
"ps" I see new
>  master  processes being created by first master process (with new workers)
>  and all of them die while new ones born.
Here I meant:

  if "preload_app" is true ...


-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El Domingo, 27 de Diciembre de 2009, Eric Wong escribi?:
> > > Hi, I''ve improved it a bit with your suggestions and
also simplified
> > > it a lot.  Now there is no a "controller process".
Instead, if
> > > "Unicorn.run" raises then master process rescues the
exception and
> > > sends USR1 to parent process so it exists with 1.
> > 
> > Hi, I realized Unicorn.run inside the daemonize method
doesn''t work.
> 
> Strange, it works for me...
"Doesn''t work" as in it''s not friendly to servers
based on Unicorn (like
Rainbows!).
> > However, I''ve reimplemented much of your idea here so it does
not
> > involve sleeping at all, but rather shares a pipe with the grandparent
> > (started by the user) and Unicorn master process.  The added benefit
of
> > using a pipe is that there''s no fuzzy sleeping involved at or
grace
> > periods to worry about, too.
> > 
> > Also pushed out to the "ready_pipe" branch of
> > git://git.bogomips.org/unicorn.git
> > 
> > Let me know how it goes.
> > 
> > If everything looks good, I''ll consider merging for 0.96.0.
> 
> It''s really awesome! I''ve tested it and it works great,
and avoids the
> sleeping stuff and a new commandlline option. Congratulations :)
The current version is actually slightly buggy in that it leaks
the pipe descriptor...
> However there is still a not solved case. Let me please explain it with two
> examples:
> 
> 1) In "after_fork" I set:
>     
worker.user("non_existing_user","non_existing_group")
> 
> The master process would start properly and would notify
"success" to
> grandparent. (so the init script returns 0). But the fact is that all the 
> workers fail to start and are respawned again and again.
For that particular case there''ll be a Unicorn::Configurator#user
directive.

But really, there''s absolutely no good reason to use user switching in
a
backend application server like Unicorn.

I only added that feature to support derivative servers like Rainbows!,
and even then it''s debatable since using things like iptables or load
balancers can be used to redirect port 80 to arbitrary ports anyways.
> 2) I use "Clogger" in my Rack config.ru. But I set a log file in
a path that
> doesn''t exist. Clogger raises due to this (it doesn''t try
to create the full
> path).
> Again the master process has notified "success" to grandparent
(exis status 0
> then) but the workers are respawned again and again.
There''s really an infinite number of ways to screw things up badly in
workers and cause them to flap.  It''s not possible to protect careless
users from all of them, so attempting to do so would be a waste of
effort.

Avoiding user-switching in an app server is a great first step, as it
eliminates an entire class of problems :)

-- 
Eric Wong

El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > The master process would start properly and would notify
"success" to
> > grandparent. (so the init script returns 0). But the fact is that all
the
> > workers fail to start and are respawned again and again.
> 
> For that particular case there''ll be a Unicorn::Configurator#user
> directive.
> 
> But really, there''s absolutely no good reason to use user
switching in a
> backend application server like Unicorn.
> 
> I only added that feature to support derivative servers like Rainbows!,
> and even then it''s debatable since using things like iptables or
load
> balancers can be used to redirect port 80 to arbitrary ports anyways.
Well, chaning the running user it''s common in most of servers.
I''ve already
found lots of cases of attacks to Apache servers running some "cool"
PHP
application (so we get exploits in /tmp or/var/tmp as they are the only 
writable paths for "www-data" user running apache).

However it''s true that Unicorn approach (worker.user) is different as
the
master process remains as root (but since the master process doesn''t
listen it
shouldn''t matter).

So, do you mean that there will be a new configuration option called
"user"
(and "group") so also themaster process would run as such user?

Thanks.

-- 
I?aki Baz Castillo <ibc at aliax.net>

El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > It''s really awesome! I''ve tested it and it works
great, and avoids the
> > sleeping stuff and a new commandlline option. Congratulations :)
> 
> The current version is actually slightly buggy in that it leaks
> the pipe descriptor...
I''ve detected some other "issue":

Imagine port 80 is used by other application (as apache) and Unicorn is 
configured to also bind in same port.


When running it in foreground all is great:

  ERROR -- : adding listener failed addr=0.0.0.0:80 (in use)
  ERROR -- : retrying in 3 seconds (1 tries left)
  ERROR -- : adding listener failed addr=0.0.0.0:80 (in use)
  ERROR -- : retrying in 3 seconds (0 tries left)
  ERROR -- : adding listener failed addr=0.0.0.0:80 (in use)
  /usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/socket_helper.rb:110:in  
  `initialize'': Address already in use - bind(2) (Errno::EADDRINUSE)


But when running in background an ugly error is displayed:

  /usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/launcher.rb:45:in
`sysread'':
  end of file reached (EOFError)
    from /usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/launcher.rb:45:in
    `daemonize!''


In both cases $? is 1 (error) but in the last case the error message is not 
very useful.

I suspect what is happening: the master tries several times to bind and after 
N retries it terminates (so it closes the pipe and grandparent gets EOFError).

As a suggestion, could the grandparent rescue such exception and display some 
kind of error message?:

  "The master couldn''t be started. Inspect the log error or run in
foreground"

Regards.


-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > > It''s really awesome! I''ve tested it and it
works great, and avoids the
> > > sleeping stuff and a new commandlline option. Congratulations :)
> > 
> > The current version is actually slightly buggy in that it leaks
> > the pipe descriptor...
> 
> I''ve detected some other "issue":
> 
> Imagine port 80 is used by other application (as apache) and Unicorn is 
> configured to also bind in same port.
> 
> 
> When running it in foreground all is great:
> 
>   ERROR -- : adding listener failed addr=0.0.0.0:80 (in use)
>   ERROR -- : retrying in 3 seconds (1 tries left)
>   ERROR -- : adding listener failed addr=0.0.0.0:80 (in use)
>   ERROR -- : retrying in 3 seconds (0 tries left)
>   ERROR -- : adding listener failed addr=0.0.0.0:80 (in use)
>   /usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/socket_helper.rb:110:in  
>   `initialize'': Address already in use - bind(2)
(Errno::EADDRINUSE)
> 
> 
> But when running in background an ugly error is displayed:
> 
>   /usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/launcher.rb:45:in
`sysread'':
>   end of file reached (EOFError)
>     from /usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/launcher.rb:45:in
>     `daemonize!''
> 
> 
> In both cases $? is 1 (error) but in the last case the error message is not
> very useful.
> 
> I suspect what is happening: the master tries several times to bind and
after
> N retries it terminates (so it closes the pipe and grandparent gets
EOFError).
> 
> As a suggestion, could the grandparent rescue such exception and display
some
> kind of error message?:
> 
>   "The master couldn''t be started. Inspect the log error or
run in foreground"
Thanks, pushed out to the ready_pipe branch of unicorn.git
>From 52eee4e424198a3c80793ee9c930fd3bb0285168 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson at yhbt.net>
Date: Mon, 28 Dec 2009 11:16:00 -0800
Subject: [PATCH] launcher: descriptive error message on startup failure
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Rather than erroring out with a non-descript EOFError,
show a warning message telling users to check the logs
instead.

Reported-by: I?aki Baz Castillo mid=200912281350.44760.ibc at aliax.net
---
 lib/unicorn/launcher.rb |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/unicorn/launcher.rb b/lib/unicorn/launcher.rb
index 0ea836b..1871420 100644
--- a/lib/unicorn/launcher.rb
+++ b/lib/unicorn/launcher.rb
@@ -42,8 +42,11 @@ class Unicorn::Launcher
 
       if grandparent == $$
         # this will block until HttpServer#join runs (or it dies)
-        master_pid = rd.readpartial(16).to_i
-        exit!(1) unless master_pid > 1
+        master_pid = (rd.readpartial(16) rescue nil).to_i
+        unless master_pid > 1
+          warn "master failed to start, check stderr log for details"
+          exit!(1)
+        end
         exit 0
       else # unicorn master process
         options[:ready_pipe] = wr
-- 
Eric Wong

El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > As a suggestion, could the grandparent rescue such exception and
display
> > some  kind of error message?:
> > 
> >   "The master couldn''t be started. Inspect the log error
or run in
> > foreground"
> 
> Thanks, pushed out to the ready_pipe branch of unicorn.git
Hi, I attach a minor patch to make the error more verbose when Unicorn cannot
listen in a socket.

For example, as normal user (not root) I try to listen in a UNIX socket (ok)
and also on TCP port 84 (permission error). Actually I would see this error log:

I, [2009-12-28T21:05:31.503533 #26137]  INFO -- : unlinking existing
socket=/tmp/unicorn.sock
I, [2009-12-28T21:05:31.504019 #26137]  INFO -- : listening on
addr=/tmp/unicorn.sock fd=3
/usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/socket_helper.rb:110:in
`initialize'': Permission denied - bind(2) (Errno::EACCES)

By inspecting the error it seems that the problem is in the UNIX socket.

With my patch the log would be:

I, [2009-12-28T21:11:18.165918 #26589]  INFO -- : unlinking existing
socket=/tmp/openxdms.sock
I, [2009-12-28T21:11:18.166228 #26589]  INFO -- : listening on
addr=/tmp/openxdms.sock fd=3
F, [2009-12-28T21:11:18.166632 #26589] FATAL -- : error adding listener
addr=0.0.0.0:84
/usr/local/lib/ruby1.9/site_ruby/1.9.1/unicorn/socket_helper.rb:110:in
`initialize'': Permission denied - bind(2) (Errno::EACCES)


I hope it''s useful. Regards.

-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > > As a suggestion, could the grandparent rescue such exception and
display
> > > some  kind of error message?:
> > > 
> > >   "The master couldn''t be started. Inspect the log
error or run in
> > > foreground"
> > 
> > Thanks, pushed out to the ready_pipe branch of unicorn.git
> 
> Hi, I attach a minor patch to make the error more verbose when Unicorn
cannot
> listen in a socket.
Hi, it seems like you didn''t attach the patch.  Attaching patches is
strongly discouraged, though, inline patches are far easier to
review/edit/apply.

-- 
Eric Wong

El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> I?aki Baz Castillo <ibc at aliax.net> wrote:
> > El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > > > As a suggestion, could the grandparent rescue such exception
and
> > > > display some  kind of error message?:
> > > >
> > > >   "The master couldn''t be started. Inspect the
log error or run in
> > > > foreground"
> > >
> > > Thanks, pushed out to the ready_pipe branch of unicorn.git
> >
> > Hi, I attach a minor patch to make the error more verbose when Unicorn
> > cannot listen in a socket.
> 
> Hi, it seems like you didn''t attach the patch.
Ops, usually my mail client (Kmail) warns me when I write tha word
"attach"
but attach nothing... not sure why it didn''t it this time.

> Attaching patches is
> strongly discouraged, though, inline patches are far easier to
> review/edit/apply.
Ok, here it''s (note that it''s done over
''ready_pipe'' branch):


--- unicorn.rb.orig     2009-12-28 21:02:47.000000000 +0100
+++ unicorn.rb  2009-12-28 21:11:12.000000000 +0100
@@ -307,6 +307,9 @@
                      "(#{tries < 0 ? ''infinite'' :
tries} tries left)"
         sleep(delay)
         retry
+      rescue => err
+        logger.fatal "error adding listener addr=#{address}"
+        raise err
       end
     end


-- 
I?aki Baz Castillo <ibc at aliax.net>

I?aki Baz Castillo <ibc at aliax.net> wrote:
> El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > I?aki Baz Castillo <ibc at aliax.net> wrote:
> > > El Lunes, 28 de Diciembre de 2009, Eric Wong escribi?:
> > > > > As a suggestion, could the grandparent rescue such
exception and
> > > > > display some  kind of error message?:
> > > > >
> > > > >   "The master couldn''t be started.
Inspect the log error or run in
> > > > > foreground"
> > > >
> > > > Thanks, pushed out to the ready_pipe branch of unicorn.git
> > >
> > > Hi, I attach a minor patch to make the error more verbose when
Unicorn
> > > cannot listen in a socket.
> > 
> > Hi, it seems like you didn''t attach the patch.
> 
> Ops, usually my mail client (Kmail) warns me when I write tha word
"attach"
> but attach nothing... not sure why it didn''t it this time.
> 
> > Attaching patches is
> > strongly discouraged, though, inline patches are far easier to
> > review/edit/apply.
> 
> Ok, here it''s (note that it''s done over
''ready_pipe'' branch):
Thanks, applied with your name and pushed out to both ready_pipe and
master in unicorn.git
> --- unicorn.rb.orig     2009-12-28 21:02:47.000000000 +0100
> +++ unicorn.rb  2009-12-28 21:11:12.000000000 +0100
It should be easier to make a commit using git and then just
"git format-patch -M" to output it, proposed commit message and all.

In the HACKING doc, I''ve been recommending people follow the
SubmittingPatches document distributed with git when submitting
patches to Unicorn:

 http://git.bogomips.org/cgit/mirrors/git.git/tree/Documentation/SubmittingPatches

-- 
Eric Wong

El Martes, 29 de Diciembre de 2009, Eric Wong escribi?:
> Thanks, applied with your name and pushed out to both ready_pipe and
> master in unicorn.git
Thanks a lot :)

 
> > --- unicorn.rb.orig     2009-12-28 21:02:47.000000000 +0100
> > +++ unicorn.rb  2009-12-28 21:11:12.000000000 +0100
> 
> It should be easier to make a commit using git and then just
> "git format-patch -M" to output it, proposed commit message and
all.
> 
> In the HACKING doc, I''ve been recommending people follow the
> SubmittingPatches document distributed with git when submitting
> patches to Unicorn:
> 
>  http://git.bogomips.org/cgit/mirrors/git.git/tree/Documentation/Submitting
> Patches
Ok, I''ll read it.
Thanks againg. 


-- 
I?aki Baz Castillo <ibc at aliax.net>