Storing Multiple Blocks for Configurator Hooks

Storing Multiple Blocks for Configurator Hooks

[Peter Giacomo Lombardo]

Please excuse if this has been discussed or already decided upon - searches came up with nothing.

Has unicorn considered allowing multiple blocks to be supplied for Configurator Hooks (e.g. after_fork et. al.)?

Currently, it seems that only one block can be specified:
https://bogomips.org/unicorn.git/tree/lib/unicorn/configurator.rb#n628

If instead this were stored as an array of blocks, gems could register their own blocks to be run when the hooks are called which would result in less end-user burden and easier on-ramping to get a running system.  

(Confronting the ActiveRecord establish_connection and Redis re-connect seem to be a rite of passage into using Unicorn)

Has this already been discussed/decided upon?

Best,
Peter

—
Peter Giacomo Lombardo
Ruby Instrumentation Lead
https://github.com/pglombardo

Instana, Inc
https://www.instana.com

Re: Storing Multiple Blocks for Configurator Hooks

[Eric Wong]

Peter Giacomo Lombardo <peter.lombardo@instana.com> wrote:
> Please excuse if this has been discussed or already decided upon - searches came up with nothing.

Nope, I don't recall this coming up on the list.

> Has unicorn considered allowing multiple blocks to be supplied for Configurator Hooks (e.g. after_fork et. al.)?

Yes, but only briefly in the earliest stages of the project.  I
rejected it since it make things more difficult to debug and
track down.

Nowadays, it could break an existing use case:

	after_fork do
	  # ... many lines
	end
	after_fork {} # nothing, temporarily disable initial block

	(I realize putting " if false" after "end" can also disable
	 the block, but I suspect many folks who configure unicorn
	 are not regularly Rubyists).

> Currently, it seems that only one block can be specified:
> https://bogomips.org/unicorn.git/tree/lib/unicorn/configurator.rb#n628
> 
> If instead this were stored as an array of blocks, gems could register their own blocks to be run when the hooks are called which would result in less end-user burden and easier on-ramping to get a running system.  

It seems like it would be a management problem if the config got
too big and were split into many pieces.  It would be extra bad
if it were difficult to know ordering (FIFO? LIFO?) and which
part of the config each part is in.

You can manage an array yourself:

	af_cb = []
	after_fork { af_cb.each(&:call) }
	af_cb << lambda { ... }
	af_cb << lambda { ... }

> (Confronting the ActiveRecord establish_connection and Redis re-connect seem to be a rite of passage into using Unicorn)

preload_app has always defaulted to "false" for this reason :)

Also, isn't a mere disconnect all that's necessary nowadays, as
connections are made on demand?  At least that's how Sequel
behaves.  I haven't touched AR/Rails in years (since a JS
engine became mandatory).

Re: Storing Multiple Blocks for Configurator Hooks

[Peter Giacomo Lombardo]

The use case I had in mind would be for gems to include the block registration in their own code and bypass entirely the Unicorn config.  And therefore bypassing entirely the need for any end user modification of the unicorn config file.

A gem could include in their code:

if defined?(::Unicorn)
  Unicorn.after_fork do
    # after fork work
  end
end

> It seems like it would be a management problem if the config got
> too big and were split into many pieces.  It would be extra bad
> if it were difficult to know ordering (FIFO? LIFO?) and which
> part of the config each part is in.

That’s a valid concern but in my experience, every gem that required an after_fork call has been an island and order independent from each other.  I’m sure there are exceptions but those would be edge cases.

Plus in all, I can only think of 6-10 common gems off-hand that require manual after_fork additions.  I think the total number of subscribers to this feature would be limited in the scope of a single application.

> 
> You can manage an array yourself:
> 
> 	af_cb = []
> 	after_fork { af_cb.each(&:call) }
> 	af_cb << lambda { ... }
> 	af_cb << lambda { … }


My reason for asking is that as a gem author, asking a user to modify their unicorn config isn’t ideal.  The best solution would be if my gem could auto register a block to be run post fork and not require the user to do anything (zero config).

>> (Confronting the ActiveRecord establish_connection and Redis re-connect seem to be a rite of passage into using Unicorn)
> 
> preload_app has always defaulted to "false" for this reason :)

Oddly I never realized that - thanks :-)  Unfortunately major orgs/gems/libs that have Unicorn instructions almost always require preload_app true. (for whatever various reasoning)

> Also, isn't a mere disconnect all that's necessary nowadays, as
> connections are made on demand?  At least that's how Sequel
> behaves.  I haven't touched AR/Rails in years (since a JS
> engine became mandatory).

Definitely true.  The harder case is for background threads and/or actor threads.  Only the calling thread is copied to the new process and others are non-existent in the new process.  after_fork is a handy hook to re-init any background/actor threads needed.

In any case, those are my thoughts.  Not critical but more of a helpful nice to have.

Best,
Peter

—
Peter Giacomo Lombardo
Ruby Instrumentation Lead
https://github.com/pglombardo

Instana, Inc
https://www.instana.com

Re: Storing Multiple Blocks for Configurator Hooks

[Eric Wong]

Peter Giacomo Lombardo <peter.lombardo@instana.com> wrote:
> The use case I had in mind would be for gems to include the
> block registration in their own code and bypass entirely the
> Unicorn config.  And therefore bypassing entirely the need for
> any end user modification of the unicorn config file.
> 
> A gem could include in their code:
> 
> if defined?(::Unicorn)
>   Unicorn.after_fork do
>     # after fork work
>   end
> end

I will not introduce a new API like this; it tends to lock
people into unicorn (something I have always been completely
opposed to).

There are plenty of other Rack servers which also fork;
and I also don't want gem authors to have to deal with dozens
of new APIs across different servers.

> My reason for asking is that as a gem author, asking a user to
> modify their unicorn config isn’t ideal.  The best solution
> would be if my gem could auto register a block to be run post
> fork and not require the user to do anything (zero config).

Agreed.  ruby-core may still decide on a common API for atfork
callbacks:

	https://bugs.ruby-lang.org/issues/5446



One workaround you can use today is detecting $$ (Process.pid)
changes at runtime.  Checking $$ is always thread-safe and
reasonably performant.

This workaround may even be better from a safety perspective:
implementing atfork callbacks is tricky to do correctly in
the presence of threads,  POSIX even got it wrong with
pthread_atfork and had to introduce "_Fork".


The main thing you need to avoid in client libraries is sending
a high-level "i-am-disconnecting" message over the socket from
multiple children.  Just close(2) the FD, and the parent can
even keep using the original socket.

So, in other words, avoid things like the "quit" command in
memcached, NNTP, etc, or any form of SSL_shutdown(3ssl);
just issue the close(2) syscall on the FD in the child.

That all depends on client libraries, of course; but it's
completely server-agnostic.

I would be more than glad to help any library authors with
clarifying and implementing this properly.  Just respond
here or Cc: me on whatever relevant mailing lists for that
library.

> >> (Confronting the ActiveRecord establish_connection and Redis re-connect seem to be a rite of passage into using Unicorn)
> > 
> > preload_app has always defaulted to "false" for this reason :)
> 
> Oddly I never realized that - thanks :-)  Unfortunately major
> orgs/gems/libs that have Unicorn instructions almost always
> require preload_app true. (for whatever various reasoning)

Yeah, it's unfortunate that apps are developed for any
particular server in mind these days.  It defeats the
point of Rack :<