about summary refs log tree commit homepage
diff options
context:
space:
mode:
authorEric Wong <normalperson@yhbt.net>2010-09-03 01:48:24 +0000
committerEric Wong <normalperson@yhbt.net>2010-11-06 09:58:43 +0800
commit7987e1a4001491f8a494f3926037f8cbee713263 (patch)
treeb51ea115e8f6866142a5b870a9862885323c9b36
parentb45bf946545496cf8d69037113533d7a58ce7e20 (diff)
downloadunicorn-7987e1a4001491f8a494f3926037f8cbee713263.tar.gz
Yes, this means even POST/PUT bodies may be kept alive,
but only if the body (and trailers) are fully-consumed.
-rw-r--r--ext/unicorn_http/global_variables.h4
-rw-r--r--ext/unicorn_http/unicorn_http.rl65
-rw-r--r--test/unit/test_http_parser.rb16
-rw-r--r--test/unit/test_http_parser_ng.rb37
4 files changed, 67 insertions, 55 deletions
diff --git a/ext/unicorn_http/global_variables.h b/ext/unicorn_http/global_variables.h
index 8377704..274f456 100644
--- a/ext/unicorn_http/global_variables.h
+++ b/ext/unicorn_http/global_variables.h
@@ -26,8 +26,6 @@ static VALUE g_http;
 static VALUE g_http_09;
 static VALUE g_http_10;
 static VALUE g_http_11;
-static VALUE g_GET;
-static VALUE g_HEAD;
 
 /** Defines common length and error messages for input length validation. */
 #define DEF_MAX_LENGTH(N, length) \
@@ -82,8 +80,6 @@ static void init_globals(void)
   DEF_GLOBAL(http_11, "HTTP/1.1");
   DEF_GLOBAL(http_10, "HTTP/1.0");
   DEF_GLOBAL(http_09, "HTTP/0.9");
-  DEF_GLOBAL(GET, "GET");
-  DEF_GLOBAL(HEAD, "HEAD");
 }
 
 #undef DEF_GLOBAL
diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
index 236fbaa..fb99f9c 100644
--- a/ext/unicorn_http/unicorn_http.rl
+++ b/ext/unicorn_http/unicorn_http.rl
@@ -18,12 +18,12 @@
 #define UH_FL_HASTRAILER 0x8
 #define UH_FL_INTRAILER 0x10
 #define UH_FL_INCHUNK  0x20
-#define UH_FL_KAMETHOD 0x40
+#define UH_FL_REQEOF 0x40
 #define UH_FL_KAVERSION 0x80
 #define UH_FL_HASHEADER 0x100
 
-/* both of these flags need to be set for keepalive to be supported */
-#define UH_FL_KEEPALIVE (UH_FL_KAMETHOD | UH_FL_KAVERSION)
+/* all of these flags need to be set for keepalive to be supported */
+#define UH_FL_KEEPALIVE (UH_FL_KAVERSION | UH_FL_REQEOF | UH_FL_HASHEADER)
 
 /* keep this small for Rainbows! since every client has one */
 struct http_parser {
@@ -79,46 +79,29 @@ static void parser_error(const char *msg)
  */
 static void hp_keepalive_connection(struct http_parser *hp, VALUE val)
 {
-  /* REQUEST_METHOD is always set before any headers */
-  if (HP_FL_TEST(hp, KAMETHOD)) {
-    if (STR_CSTR_CASE_EQ(val, "keep-alive")) {
-      /* basically have HTTP/1.0 masquerade as HTTP/1.1+ */
-      HP_FL_SET(hp, KAVERSION);
-    } else if (STR_CSTR_CASE_EQ(val, "close")) {
-      /*
-       * it doesn't matter what HTTP version or request method we have,
-       * if a client says "Connection: close", we disable keepalive
-       */
-      HP_FL_UNSET(hp, KEEPALIVE);
-    } else {
-      /*
-       * client could've sent anything, ignore it for now.  Maybe
-       * "HP_FL_UNSET(hp, KEEPALIVE);" just in case?
-       * Raising an exception might be too mean...
-       */
-    }
+  if (STR_CSTR_CASE_EQ(val, "keep-alive")) {
+    /* basically have HTTP/1.0 masquerade as HTTP/1.1+ */
+    HP_FL_SET(hp, KAVERSION);
+  } else if (STR_CSTR_CASE_EQ(val, "close")) {
+    /*
+     * it doesn't matter what HTTP version or request method we have,
+     * if a client says "Connection: close", we disable keepalive
+     */
+    HP_FL_UNSET(hp, KAVERSION);
+  } else {
+    /*
+     * client could've sent anything, ignore it for now.  Maybe
+     * "HP_FL_UNSET(hp, KAVERSION);" just in case?
+     * Raising an exception might be too mean...
+     */
   }
 }
 
 static void
 request_method(struct http_parser *hp, const char *ptr, size_t len)
 {
-  VALUE v;
+  VALUE v = rb_str_new(ptr, len);
 
-  /*
-   * we only support keepalive for GET and HEAD requests for now other
-   * methods are too rarely seen to be worth optimizing.  POST is unsafe
-   * since some clients send extra bytes after POST bodies.
-   */
-  if (CONST_MEM_EQ("GET", ptr, len)) {
-    HP_FL_SET(hp, KAMETHOD);
-    v = g_GET;
-  } else if (CONST_MEM_EQ("HEAD", ptr, len)) {
-    HP_FL_SET(hp, KAMETHOD);
-    v = g_HEAD;
-  } else {
-    v = rb_str_new(ptr, len);
-  }
   rb_hash_aset(hp->env, g_request_method, v);
 }
 
@@ -206,7 +189,8 @@ static void write_value(struct http_parser *hp,
     hp->len.content = parse_length(RSTRING_PTR(v), RSTRING_LEN(v));
     if (hp->len.content < 0)
       parser_error("invalid Content-Length");
-    HP_FL_SET(hp, HASBODY);
+    if (hp->len.content != 0)
+      HP_FL_SET(hp, HASBODY);
     hp_invalid_if_trailer(hp);
   } else if (f == g_http_transfer_encoding) {
     if (STR_CSTR_CASE_EQ(v, "chunked")) {
@@ -305,6 +289,7 @@ static void write_value(struct http_parser *hp,
       if (HP_FL_TEST(hp, CHUNKED))
         cs = http_parser_en_ChunkedBody;
     } else {
+      HP_FL_SET(hp, REQEOF);
       assert(!HP_FL_TEST(hp, CHUNKED) && "chunked encoding without body!");
     }
     /*
@@ -559,6 +544,8 @@ static VALUE HttpParser_parse(VALUE self)
       hp->cs == http_parser_en_ChunkedBody) {
     advance_str(data, hp->offset + 1);
     hp->offset = 0;
+    if (HP_FL_TEST(hp, INTRAILER))
+      HP_FL_SET(hp, REQEOF);
 
     return hp->env;
   }
@@ -710,8 +697,10 @@ static VALUE HttpParser_filter_body(VALUE self, VALUE buf, VALUE data)
 
       memcpy(RSTRING_PTR(buf), dptr, nr);
       hp->len.content -= nr;
-      if (hp->len.content == 0)
+      if (hp->len.content == 0) {
+        HP_FL_SET(hp, REQEOF);
         hp->cs = http_parser_first_final;
+      }
       advance_str(data, nr);
       rb_str_set_len(buf, nr);
       data = Qnil;
diff --git a/test/unit/test_http_parser.rb b/test/unit/test_http_parser.rb
index 222c227..31cb2cb 100644
--- a/test/unit/test_http_parser.rb
+++ b/test/unit/test_http_parser.rb
@@ -76,12 +76,22 @@ class HttpParserTest < Test::Unit::TestCase
     assert parser.keepalive?
   end
 
-  def test_connection_keep_alive_ka_bad_method
+  def test_connection_keep_alive_no_body
     parser = HttpParser.new
     req = {}
     tmp = "POST / HTTP/1.1\r\nConnection: keep-alive\r\n\r\n"
     assert_equal req.object_id, parser.headers(req, tmp).object_id
-    assert ! parser.keepalive?
+    assert parser.keepalive?
+  end
+
+  def test_connection_keep_alive_no_body_empty
+    parser = HttpParser.new
+    req = {}
+    tmp = "POST / HTTP/1.1\r\n" \
+          "Content-Length: 0\r\n" \
+          "Connection: keep-alive\r\n\r\n"
+    assert_equal req.object_id, parser.headers(req, tmp).object_id
+    assert parser.keepalive?
   end
 
   def test_connection_keep_alive_ka_bad_version
@@ -461,7 +471,7 @@ class HttpParserTest < Test::Unit::TestCase
       assert_equal 'page=1', req['QUERY_STRING']
       assert_equal "", s
       assert_equal m, req['REQUEST_METHOD']
-      assert ! parser.keepalive? # TODO: read HTTP/1.2 when it's final
+      assert parser.keepalive? # TODO: read HTTP/1.2 when it's final
     }
   end
 
diff --git a/test/unit/test_http_parser_ng.rb b/test/unit/test_http_parser_ng.rb
index 65b843e..699ee64 100644
--- a/test/unit/test_http_parser_ng.rb
+++ b/test/unit/test_http_parser_ng.rb
@@ -27,6 +27,12 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert ! @parser.keepalive?
     assert @parser.headers?
     assert_equal 123, @parser.content_length
+    dst = ""
+    buf = '.' * 123
+    @parser.filter_body(dst, buf)
+    assert_equal '.' * 123, dst
+    assert_equal "", buf
+    assert @parser.keepalive?
   end
 
   def test_identity_step_headers
@@ -41,6 +47,12 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal 0, str.size
     assert ! @parser.keepalive?
     assert @parser.headers?
+    dst = ""
+    buf = '.' * 123
+    @parser.filter_body(dst, buf)
+    assert_equal '.' * 123, dst
+    assert_equal "", buf
+    assert @parser.keepalive?
   end
 
   def test_identity_oneshot_header
@@ -50,6 +62,12 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal '123', req['CONTENT_LENGTH']
     assert_equal 0, str.size
     assert ! @parser.keepalive?
+    assert @parser.headers?
+    dst = ""
+    buf = '.' * 123
+    @parser.filter_body(dst, buf)
+    assert_equal '.' * 123, dst
+    assert_equal "", buf
   end
 
   def test_identity_oneshot_header_with_body
@@ -67,7 +85,7 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal 0, str.size
     assert_equal tmp, body
     assert_equal "", @parser.filter_body(tmp, str)
-    assert ! @parser.keepalive?
+    assert @parser.keepalive?
   end
 
   def test_identity_oneshot_header_with_body_partial
@@ -85,7 +103,7 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_nil rv
     assert_equal "", str
     assert_equal str.object_id, @parser.filter_body(tmp, str).object_id
-    assert ! @parser.keepalive?
+    assert @parser.keepalive?
   end
 
   def test_identity_oneshot_header_with_body_slop
@@ -99,7 +117,7 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal "G", @parser.filter_body(tmp, str)
     assert_equal 1, tmp.size
     assert_equal "a", tmp
-    assert ! @parser.keepalive?
+    assert @parser.keepalive?
   end
 
   def test_chunked
@@ -122,6 +140,10 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal rv.object_id, @parser.filter_body(tmp, rv).object_id
     assert_equal "PUT", rv
     assert ! @parser.keepalive?
+    rv << "TY: FOO\r\n\r\n"
+    assert_equal req, @parser.trailers(req, rv)
+    assert_equal "FOO", req["HTTP_PUTTY"]
+    assert @parser.keepalive?
   end
 
   def test_two_chunks
@@ -177,7 +199,7 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal req, @parser.trailers(req, moo = "\r\n")
     assert_equal "", moo
     assert @parser.body_eof?
-    assert ! @parser.keepalive?
+    assert @parser.keepalive?
   end
 
   def test_two_chunks_oneshot
@@ -237,7 +259,7 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_nil @parser.trailers(req, str << "\r")
     assert_equal req, @parser.trailers(req, str << "\nGET / ")
     assert_equal "GET / ", str
-    assert ! @parser.keepalive?
+    assert @parser.keepalive?
   end
 
   def test_trailers_slowly
@@ -297,14 +319,12 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal req, @parser.headers(req, str)
     assert_nil @parser.content_length
     assert_raise(HttpParserError) { @parser.filter_body('', str) }
-    assert ! @parser.keepalive?
   end
 
   def test_overflow_content_length
     n = HttpParser::LENGTH_MAX + 1
     str = "PUT / HTTP/1.1\r\nContent-Length: #{n}\r\n\r\n"
     assert_raise(HttpParserError) { @parser.headers({}, str) }
-    assert ! @parser.keepalive?
   end
 
   def test_bad_chunk
@@ -315,13 +335,11 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal req, @parser.headers(req, str)
     assert_nil @parser.content_length
     assert_raise(HttpParserError) { @parser.filter_body('', str) }
-    assert ! @parser.keepalive?
   end
 
   def test_bad_content_length
     str = "PUT / HTTP/1.1\r\nContent-Length: 7ff\r\n\r\n"
     assert_raise(HttpParserError) { @parser.headers({}, str) }
-    assert ! @parser.keepalive?
   end
 
   def test_bad_trailers
@@ -338,7 +356,6 @@ class HttpParserNgTest < Test::Unit::TestCase
     assert_equal '', str
     str << "Transfer-Encoding: identity\r\n\r\n"
     assert_raise(HttpParserError) { @parser.trailers(req, str) }
-    assert ! @parser.keepalive?
   end
 
   def test_repeat_headers