From mboxrd@z Thu Jan 1 00:00:00 1970 Delivered-To: chneukirchen@gmail.com Received: by 10.140.141.15 with SMTP id o15cs4433rvd; Tue, 5 Jan 2010 15:58:50 -0800 (PST) Received: from mr.google.com ([10.141.124.18]) by 10.141.124.18 with SMTP id b18mr17945143rvn.5.1262735929644 (num_hops = 1); Tue, 05 Jan 2010 15:58:49 -0800 (PST) Received: by 10.141.124.18 with SMTP id b18mr3398863rvn.5.1262735928267; Tue, 05 Jan 2010 15:58:48 -0800 (PST) X-BeenThere: rack-devel@googlegroups.com Received: by 10.141.100.2 with SMTP id c2ls246940rvm.1.p; Tue, 05 Jan 2010 15:58:47 -0800 (PST) Received: by 10.140.83.9 with SMTP id g9mr5435029rvb.11.1262735926817; Tue, 05 Jan 2010 15:58:46 -0800 (PST) Received: by 10.140.83.9 with SMTP id g9mr5435028rvb.11.1262735926796; Tue, 05 Jan 2010 15:58:46 -0800 (PST) Return-Path: Received: from dcvr.yhbt.net (dcvr.yhbt.net [64.71.152.64]) by gmr-mx.google.com with ESMTP id 23si7293611pxi.0.2010.01.05.15.58.46; Tue, 05 Jan 2010 15:58:46 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of normalperson@yhbt.net designates 64.71.152.64 as permitted sender) client-ip=64.71.152.64; Received: from localhost (unknown [127.0.2.5]) by dcvr.yhbt.net (Postfix) with ESMTP id 04CDE1F4E5; Tue, 5 Jan 2010 23:58:45 +0000 (UTC) Date: Tue, 5 Jan 2010 15:58:45 -0800 From: Eric Wong To: rack-devel@googlegroups.com Subject: Re: [PATCH 0/3] support reuse of HeaderHash objects Message-ID: <20100105235845.GB3377@dcvr.yhbt.net> References: <1252100514-2748-1-git-send-email-normalperson@yhbt.net> MIME-Version: 1.0 In-Reply-To: <1252100514-2748-1-git-send-email-normalperson@yhbt.net> User-Agent: Mutt/1.5.18 (2008-05-17) X-Original-Authentication-Results: gmr-mx.google.com; spf=pass (google.com: best guess record for domain of normalperson@yhbt.net designates 64.71.152.64 as permitted sender) smtp.mail=normalperson@yhbt.net X-Original-Sender: normalperson@yhbt.net Reply-To: rack-devel@googlegroups.com Precedence: list Mailing-list: list rack-devel@googlegroups.com; contact rack-devel+owners@googlegroups.com List-ID: List-Post: , List-Help: , List-Archive: X-Thread-Url: http://groups.google.com/group/rack-devel/t/e5ceb39994b71080 X-Message-Url: http://groups.google.com/group/rack-devel/msg/64e4c5840ad8ed2 Sender: rack-devel@googlegroups.com List-Unsubscribe: , List-Subscribe: , Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Eric Wong wrote: > HeaderHash objects are needlessly created and discarded by various > pieces of Rack middleware, leading to unnecessary overhead > from both object creation + GC and iteration overhead via > headers#each. Ugh, this series interacts badly with applications and frameworks that set cookies without using Rack::Utils. Rails 2.3.5 is one (popular) example of a framework that sets cookies without help from Rack::Utils. Below is a patch (against Rails 2.3.5) that pinpoints and fixes the problematic code: (I don't expect this patch to be applied) diff --git a/actionpack/lib/action_controller/session/abstract_store.rb b/actionpack/lib/action_controller/session/abstract_store.rb index f6369ab..2df45d8 100644 --- a/actionpack/lib/action_controller/session/abstract_store.rb +++ b/actionpack/lib/action_controller/session/abstract_store.rb @@ -144,8 +144,9 @@ module ActionController cookie << "; HttpOnly" if options[:httponly] headers = response[1] - unless headers[SET_COOKIE].blank? - headers[SET_COOKIE] << "\n#{cookie}" + set_cookie = headers[SET_COOKIE] + unless set_cookie.blank? + set_cookie << (Array === set_cookie ? cookie : "\n#{cookie}") else headers[SET_COOKIE] = cookie end Without the above patch, I end up with a blank cookie with nothing in it when (using Unicorn): Set-Cookie: set_by_app=cookie-1715-; path=/ Set-Cookie: Set-Cookie: _session_cookie=98e3f12545e70a6b7a943628a3a7578b; path=/; HttpOnly This was because Rails was blindly prepending "\n" to the cookie thinking it's hitting a string, but the value of headers[SET_COOKIE] is now an array... Below is a potential fix to Rack to fix the problem, but I'm not sure about the severity of this problem with real HTTP clients (other than 12 bytes). diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb index 68fd6ac..10dc8fe 100644 --- a/lib/rack/utils.rb +++ b/lib/rack/utils.rb @@ -273,19 +273,23 @@ module Rack hash.each { |k, v| self[k] = v } end + def value_string(v) + if v.respond_to?(:to_ary) + v = v.to_ary.join("\n") + v.gsub!(/\n\n+/, "\n") + end + v + end + def each super do |k, v| - yield(k, v.respond_to?(:to_ary) ? v.to_ary.join("\n") : v) + yield(k, value_string(v)) end end def to_hash inject({}) do |hash, (k,v)| - if v.respond_to? :to_ary - hash[k] = v.to_ary.join("\n") - else - hash[k] = v - end + hash[k] = value_string(v) hash end end --- There's also a one-line fix that I could make to Unicorn by replacing split(/\n/) with split(/\n+/), which may be a good idea anyways in case frameworks/apps attempt more "creative" ways to set other headers... -- Eric Wong