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: Mon, 5 Nov 2012 11:48:50 +0000
Message-ID: <20121105114850.GA15932@dcvr.yhbt.net> (raw)
In-Reply-To: <CAK4qKG1AJdsXvTuUT+Q85qTZzfCTLUMDd=d1LzFgfM6THc5G=g@mail.gmail.com>

Tom Burns <tom.burns@jadedpixel.com> wrote:
> On Fri, Nov 2, 2012 at 3:38 PM, Eric Wong <normalperson@yhbt.net> wrote:
> > Everything looks good for stock configurations.  The ERROR_XXX_RESPONSE
> > constants should have "HTTP/1.1 " removed if used with this option, too.
> > That requires keeping track of whether or not we've written out
> > "HTTP/1.1 ", yet.
> 
> The patch below tracks if we've sent the start of the response.  It
> also passes that to http_response_write as a parameter with default
> value false.  By doing this, the test suite passes if you change the
> default value for check_client_connection to true, which makes me feel
> a lot better about the patch.

Cool.

> > > The only thing it's missing from your TODO is enforcing
> > > tcp_nodelay/tcp_nopush as I wasn't sure where was the best place to do
> > > the enforcement.
> >
> > It should probably be done inside Unicorn::Configurator#commit!
> 
> So, I still didn't so this, because in commit! we don't know if it's a
> TCP or UNIX socket, and I don't think it'd be nice to force tcp
> configs if the user's using a UNIX socket.  If you disagree I can add
> the check there, it just seemed weird when I looked at it.

Erm, just check if :tcp_* is set in the :listener_opts hash and raise if
there's incompatible settings.  We shouldn't be changing settings behind
users' backs.

> >From d7e249202b0fd5c24f2b98c700e02bf992c6576b Mon Sep 17 00:00:00 2001
> From: Tom Burns <tom.burns@jadedpixel.com>
> Date: Tue, 30 Oct 2012 16:22:21 -0400
> Subject: [PATCH] Begin writing HTTP request headers early to detect
>  disconnected clients

This definitely needs a thorough commit message which explains why this
is a necessary addition, how it works, when it works, and what benefits
it brings.

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 :>

> --- a/lib/unicorn/http_request.rb
> +++ b/lib/unicorn/http_request.rb
> @@ -70,6 +82,15 @@ class Unicorn::HttpParser
>        # an Exception thrown from the parser will throw us out of the loop
>        false until add_parse(socket.kgio_read!(16384))
>      end
> +
> +    # detect if the socket is valid by writing a partial response:
> +    if @@check_client_connection && headers?
> +      @response_start_sent = true
> +      Unicorn::Const::HTTP_RESPONSE_START.each { |c| socket.write(c) }
> +    else
> +      @response_start_sent = false

This else portion is redundant given it's set to false when you reset
the parser (see my final comment below)

> --- a/lib/unicorn/http_server.rb
> +++ b/lib/unicorn/http_server.rb
> @@ -524,23 +533,33 @@ class Unicorn::HttpServer
>        Unicorn.log_error(@logger, "app error", e)
>        Unicorn::Const::ERROR_500_RESPONSE
>      end
> +    msg = 'HTTP/1.1 ' + msg unless @request.response_start_sent

String interpolation would be easier to read, here (and slightly
smaller bytecode if it matters :>).

>      client.kgio_trywrite(msg)
>      client.close
>      rescue
>    end
> 
> +  def expect_100_response
> +    if @request.response_start_sent
> +      Unicorn::Const::EXPECT_100_RESPONSE_SUFFIXED
> +    else
> +      Unicorn::Const::EXPECT_100_RESPONSE
> +    end
> +  end
> +
>    # once a client is accepted, it is processed in its entirety here
>    # in 3 easy steps: read request, call app, write app response
>    def process_client(client)
> +    @request.response_start_sent = false

Better to reset this ivar in the HttpParser#reset method
(it's in the C extension, I can change it later).

Random thought:  HttpResponse generation gains even more coupling
with the current Http{Request,Parser} state.  Organizing code is hard :x
_______________________________________________
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 [this message]
2012-11-06  3:16                 ` Tom Burns
2012-11-06 21:23                   ` Eric Wong
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=20121105114850.GA15932@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