-1 shutdown_timeout?

-1 shutdown_timeout?

[Lin Jen-Shin (godfat)]

Hi,

I am trying to switch one application from using rainbows
to yahns, and the application would do some long-polling.

When I ctrl+c to interrupt yahns, I would see yahns is
trying to drop clients which still connect, with timeout
set to 5 (from my config setting client_timeout to 5).

However, after a few seconds, there would be messages
like this from yahns:

    dropping 0 of 1 clients for timeout=-1

The timeout would be -1 after a few seconds, and then
it would only stop when those clients timeout at 60 seconds
(this is my assumption though, it would show a request was
 around 60 seconds from CommonLogger (assumption again)
 in the end, and yahns would quit gracefully)

I looked around the code, which should be:

  def dropping(fdmap)
    if drop_acceptors[0] || fdmap.size > 0
      timeout = @shutdown_expire < Yahns.now ? -1 : @shutdown_timeout
      fdmap.desperate_expire(timeout)
      true
    else
      false
    end
  end

While Yahns.now would get growing, I don't really understand why
it's written like this? Is this intentional? If so, how could I force
yahns shut those clients down in this case?

Thanks!

Re: -1 shutdown_timeout?

[Eric Wong]

"Lin Jen-Shin (godfat)" <godfat@godfat.org> wrote:
> Hi,
> 
> I am trying to switch one application from using rainbows
> to yahns, and the application would do some long-polling.
> 
> When I ctrl+c to interrupt yahns, I would see yahns is
> trying to drop clients which still connect, with timeout
> set to 5 (from my config setting client_timeout to 5).
> 
> However, after a few seconds, there would be messages
> like this from yahns:
> 
>     dropping 0 of 1 clients for timeout=-1

-1 means gently kill off every socket that can be killed
(inside fdmap.rb __expire).

But the shutdown tries to be gentle: it merely makes the socket
unusable to the worker thread.  It has no way to interrupt a worker
thread.

> The timeout would be -1 after a few seconds, and then
> it would only stop when those clients timeout at 60 seconds
> (this is my assumption though, it would show a request was
>  around 60 seconds from CommonLogger (assumption again)
>  in the end, and yahns would quit gracefully)

So your long request taking 60s is normal/expected?  yahns
*should* be waiting that long (unless you send another SIGQUIT).

You can probably set a shutdown_timeout >= 60s to quiet down the log
messages.  It defaults to the highest client_timeout of all the apps
running.

> I looked around the code, which should be:
> 
>   def dropping(fdmap)
>     if drop_acceptors[0] || fdmap.size > 0
>       timeout = @shutdown_expire < Yahns.now ? -1 : @shutdown_timeout
>       fdmap.desperate_expire(timeout)
>       true
>     else
>       false
>     end
>   end
>
> While Yahns.now would get growing, I don't really understand why
> it's written like this? Is this intentional? If so, how could I force
> yahns shut those clients down in this case?

Rereading the code, it seems intentional and correct.  Setting the -1
timeout, means the shutdown_timeout has expired.  yahns will not raise
cross-thread exceptions or signals to interrupt apps, so I guess the use
of the word "desperate" is a misnomer.

Admittedly, the shutdown code is a bit hairy and it took me to a bit to
fix some of the race conditions back in 2013.

Re: -1 shutdown_timeout?

[Lin Jen-Shin (godfat)]

On Wed, Sep 23, 2015 at 4:05 PM, Eric Wong <e@80x24.org> wrote:
> "Lin Jen-Shin (godfat)" <godfat@godfat.org> wrote:
[...]
>> The timeout would be -1 after a few seconds, and then
>> it would only stop when those clients timeout at 60 seconds
>> (this is my assumption though, it would show a request was
>>  around 60 seconds from CommonLogger (assumption again)
>>  in the end, and yahns would quit gracefully)
>
> So your long request taking 60s is normal/expected?  yahns
> *should* be waiting that long (unless you send another SIGQUIT).
>
> You can probably set a shutdown_timeout >= 60s to quiet down the log
> messages.  It defaults to the highest client_timeout of all the apps
> running.

Looking at the code again (it's written long ago), yes you're right,
the polling would stop at 60s if there's no data available.
So I guess this is quite expected behaviour.

However instead of setting shutdown_timeout, I think I would probably
need to make the application know that the server is shutting down,
and stop right there instead of waiting for full 60 seconds.

Not sure if I could do this without talking to yahns directly...

>> I looked around the code, which should be:
>>
>>   def dropping(fdmap)
>>     if drop_acceptors[0] || fdmap.size > 0
>>       timeout = @shutdown_expire < Yahns.now ? -1 : @shutdown_timeout
>>       fdmap.desperate_expire(timeout)
>>       true
>>     else
>>       false
>>     end
>>   end
>>
>> While Yahns.now would get growing, I don't really understand why
>> it's written like this? Is this intentional? If so, how could I force
>> yahns shut those clients down in this case?
>
> Rereading the code, it seems intentional and correct.  Setting the -1
> timeout, means the shutdown_timeout has expired.  yahns will not raise
> cross-thread exceptions or signals to interrupt apps, so I guess the use
> of the word "desperate" is a misnomer.
>
> Admittedly, the shutdown code is a bit hairy and it took me to a bit to
> fix some of the race conditions back in 2013.

Understood now. Thank you!

Re: -1 shutdown_timeout?

[Eric Wong]

"Lin Jen-Shin (godfat)" <godfat@godfat.org> wrote:
> However instead of setting shutdown_timeout, I think I would probably
> need to make the application know that the server is shutting down,
> and stop right there instead of waiting for full 60 seconds.
> 
> Not sure if I could do this without talking to yahns directly...

This is a Rack app?  I really don't want to introduce custom APIs for
this which would lock Rack users down to yahns.

I suppose you could chain signal handlers, though:

  orig_quit = nil
  orig_quit = trap(:QUIT) do
    app_specific_quit
    orig_quit.call
  end

(Ditto for TERM/INT, totally untested)

Re: -1 shutdown_timeout?

[Lin Jen-Shin (godfat)]

On Wed, Sep 23, 2015 at 4:30 PM, Eric Wong <e@80x24.org> wrote:
> "Lin Jen-Shin (godfat)" <godfat@godfat.org> wrote:
>> However instead of setting shutdown_timeout, I think I would probably
>> need to make the application know that the server is shutting down,
>> and stop right there instead of waiting for full 60 seconds.
>>
>> Not sure if I could do this without talking to yahns directly...
>
> This is a Rack app?  I really don't want to introduce custom APIs for
> this which would lock Rack users down to yahns.

Yes this is a Rack app. I totally understand locking down to a specific
webserver would not be so good, but sometimes I really want to
make the best tweak so I'm personally ok with some specific
configuration, especially given that I realized switching to a different
server could mean some other configuration should be changed as well.

The most recent example is database connection pool size.
With a threading server, I could set the size to a very large number,
but for a forking server like Unicorn, the size should be much
lower. So to switch servers, we already need to make a few changes
accordingly. (detecting would be even more works :()

> I suppose you could chain signal handlers, though:
>
>   orig_quit = nil
>   orig_quit = trap(:QUIT) do
>     app_specific_quit
>     orig_quit.call
>   end
>
> (Ditto for TERM/INT, totally untested)

Just tried it, but not sure where to put this.
With preload=true, yahns is trapping later than
the application. Maybe I should just set it to false
since we're not forking anyway...

Re: -1 shutdown_timeout?

[Eric Wong]

"Lin Jen-Shin (godfat)" <godfat@godfat.org> wrote:
> Just tried it, but not sure where to put this.
> With preload=true, yahns is trapping later than
> the application. Maybe I should just set it to false
> since we're not forking anyway...

Anywhere in your normal app dispatch path, just initialize the
lock during application load (regardless of preload value)

  def initialize(...)
    @lock = Mutex.new
    @old_quit = nil
  end

  def call(env)
    unless @old_quit
      @lock.synchronize do
        @old_quit ||= trap(:QUIT) do
          app_specific_quit
          @old_quit.call
        end
      end
    end
    ...
  end

Re: -1 shutdown_timeout?

[Lin Jen-Shin (godfat)]

On Wed, Sep 23, 2015 at 5:31 PM, Eric Wong <e@80x24.org> wrote:
> "Lin Jen-Shin (godfat)" <godfat@godfat.org> wrote:
>> Just tried it, but not sure where to put this.
>> With preload=true, yahns is trapping later than
>> the application. Maybe I should just set it to false
>> since we're not forking anyway...
>
> Anywhere in your normal app dispatch path, just initialize the
> lock during application load (regardless of preload value)
>
>   def initialize(...)
>     @lock = Mutex.new
>     @old_quit = nil
>   end
>
>   def call(env)
>     unless @old_quit
>       @lock.synchronize do
>         @old_quit ||= trap(:QUIT) do
>           app_specific_quit
>           @old_quit.call
>         end
>       end
>     end
>     ...
>   end

Oh, I really hope we don't have to initialize/load anything
during processing requests. But I guess this is the only
way to make sure this is compatible between different
servers... :(

I really hope rack warmup were implemented the other
way so that we could initialize this while warming up...
Like the server should be calling it before serving request,
rather than at the time application was loaded.