unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / Atom feed
From: Eric Wong <normalperson@yhbt.net>
To: unicorn list <mongrel-unicorn@rubyforge.org>
Subject: Re: Combating nginx 499 HTTP responses during flash traffic scenario
Date: Tue, 6 Nov 2012 21:23:38 +0000
Message-ID: <20121106212338.GA4018@dcvr.yhbt.net> (raw)
In-Reply-To: <CAK4qKG302+i7+WLnOqc=1FHi4dSEUd_j7J=JJjcLOsFWObncKQ@mail.gmail.com>

Tom Burns <tom.burns@jadedpixel.com> wrote:
> On Mon, Nov 5, 2012 at 6:48 AM, Eric Wong <normalperson@yhbt.net> wrote:
> > We can wait until you can report on how this change improves your
> > situation before fleshing this out.  Real-world results are always
> > good to have :>
> 
> Agreed.
> 
> We're going to be testing this this week so I should have some results
> to share by the weekend.  We only experience the problem during major
> sales so it may or may not manifest without our traffic generation
> tool.

Holidays are coming up, should be lots of traffic :)

> I'd like to be testing a patch close to the finished product so I've
> addressed all of your comments from the previous email (including the
> modification to the C extension) in the patch below.

Thanks for the updated patch.  A few comments below, apologies for the
nitpicks :)

> > Random thought:  HttpResponse generation gains even more coupling
> > with the current Http{Request,Parser} state.  Organizing code is hard :x
> Agreed.  Without a class wrapping the raw socket to hold state I
> didn't see a better way to accomplish this, but I've only been looking
> at this code for the past week or so.

I'm leaning towards just having one HttpState class which encompasses
the entire client state (request/response).  I did that earlier this
year for a single-purpose (non-Ruby) HTTP daemon and that design makes
the most sense to me.

Maybe we'll modify unicorn around that sooner or later...

> diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
> index 96dcf83..52f7f3c 100644
> --- a/ext/unicorn_http/unicorn_http.rl
> +++ b/ext/unicorn_http/unicorn_http.rl
> @@ -115,7 +115,7 @@ struct http_parser {
>    } len;
>  };
> 
> -static ID id_clear, id_set_backtrace;
> +static ID id_clear, id_set_backtrace, id_response_start_sent;
> 
>  static void finalize_header(struct http_parser *hp);
> 
> @@ -626,6 +626,7 @@ static VALUE HttpParser_clear(VALUE self)
> 
>    http_parser_init(hp);
>    rb_funcall(hp->env, id_clear, 0);
> +  rb_funcall(self, id_response_start_sent, 1, Qfalse);

Nitpick:  Since HttpParser is already an alias of the HttpRequest class,
rb_ivar_set() should be a hair faster as it won't have to go through
normal method lookup.

>    return self;
>  }
> @@ -1031,6 +1032,7 @@ void Init_unicorn_http(void)
>    SET_GLOBAL(g_http_connection, "CONNECTION");
>    id_clear = rb_intern("clear");
>    id_set_backtrace = rb_intern("set_backtrace");
> +  id_response_start_sent = rb_intern("response_start_sent=");
>    init_unicorn_httpdate();
>  }
>  #undef SET_GLOBAL
> diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
> index 89cbf5c..5f329af 100644
> --- a/lib/unicorn/configurator.rb
> +++ b/lib/unicorn/configurator.rb
> @@ -45,6 +45,7 @@ class Unicorn::Configurator
>        },
>      :pid => nil,
>      :preload_app => false,
> +    :check_client_connection => false,
>      :rewindable_input => true, # for Rack 2.x: (Rack::VERSION[0] <= 1),
>      :client_body_buffer_size => Unicorn::Const::MAX_BODY,
>      :trust_x_forwarded => true,
> @@ -96,6 +97,13 @@ class Unicorn::Configurator
>      if ready_pipe = RACKUP.delete(:ready_pipe)
>        server.ready_pipe = ready_pipe
>      end
> +    if set[:check_client_connection]
> +      set[:listeners].each do |address|
> +        if set[:listener_opts][address][:tcp_nopush] == true ||
> set[:listener_opts][address][:tcp_nodelay] == true

Oops, I missed long lines the first time.  It looks like your MUA did
mangle the long line.  Wrap everything at <80 columns should help avoid
the issue (regardless of MUA, my brain cannot process >80 columns well)

> +          raise ArgumentError, "With check_client_connection set to
> true both :tcp_nopush and :tcp_nodelay listener options must be set to
> false."

Likewise, error messages should be more concise and easier to read
in smaller terminals.

Probably:

check_client_connection is incompatible with (tcp_nopush|tcp_nodelay):true

> @@ -454,6 +462,12 @@ class Unicorn::Configurator
>      set_int(:client_body_buffer_size, bytes, 0)
>    end
> 
> +  # When enabled, unicorn will check the client connection by writing
> +  # the beginning of the HTTP headers before calling the application.

Good to document the :tcp_* listener option incompatibilities here, too.

> +  def check_client_connection(bool)
> +    set_bool(:check_client_connection, bool)
> +  end
> +
_______________________________________________
Unicorn mailing list - mongrel-unicorn@rubyforge.org
http://rubyforge.org/mailman/listinfo/mongrel-unicorn
Do not quote signatures (like this one) or top post when replying

  reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-29 17:44 Tom Burns
2012-10-29 18:45 ` Eric Wong
2012-10-29 19:27   ` Hongli Lai
2012-10-29 19:41     ` Eric Wong
2012-10-29 21:06       ` Hongli Lai
2012-10-29 21:53   ` Eric Wong
2012-10-29 22:21     ` Tom Burns
2012-10-30 20:40     ` Tom Burns
2012-10-30 21:37       ` Eric Wong
2012-11-02 17:59         ` Tom Burns
2012-11-02 19:38           ` Eric Wong
2012-11-03 22:45             ` Tom Burns
2012-11-05 11:48               ` Eric Wong
2012-11-06  3:16                 ` Tom Burns
2012-11-06 21:23                   ` Eric Wong [this message]
2012-11-29 15:52                     ` Tom Burns
2012-11-29 20:30                       ` Lawrence Pit
2012-11-29 20:57                         ` Tom Burns
2012-11-29 21:30                         ` Eric Wong
2012-11-30 23:47                           ` Eric Wong
2012-11-29 20:41                       ` Eric Wong
2012-12-04  3:00                         ` Eric Wong
2012-11-29 21:19                       ` Eric Wong
2012-11-29 21:55                         ` [RFC/PATCH] check_client_connection: document local-only requirement Eric Wong
2012-11-29 23:47                           ` Tom Burns

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://bogomips.org/unicorn/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121106212338.GA4018@dcvr.yhbt.net \
    --to=normalperson@yhbt.net \
    --cc=mongrel-unicorn@rubyforge.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help

Archives are clonable:
	git clone --mirror https://bogomips.org/unicorn-public
	git clone --mirror http://ou63pmih66umazou.onion/unicorn-public

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.lang.ruby.unicorn
	nntp://ou63pmih66umazou.onion/inbox.comp.lang.ruby.unicorn

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox