rack 3.x compatibility

rack 3.x compatibility

[Stefan Sundin]

Hello. I did some work to get clogger up to spec with rack 3.x.

This is my first time using git to send patches, so hopefully
I did everything right. Feels good to not use GitHub for a change.

The only "breakage" here is that malformed headers will now
throw an error instead of work. Let me know if you prefer
another solution other than deleting the test
test_broken_header_response.

Thanks!

[PATCH 1/2] update deprecated license identifier

[Stefan Sundin]

---
 clogger.gemspec | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clogger.gemspec b/clogger.gemspec
index d461383..ab479c5 100644
--- a/clogger.gemspec
+++ b/clogger.gemspec
@@ -19,5 +19,5 @@ Gem::Specification.new do |s|
   s.add_dependency(%q<rack>, ['>= 1.0', '< 3.0'])
   s.add_development_dependency('test-unit', '~> 3.0')
   s.extensions = %w(ext/clogger_ext/extconf.rb)
-  s.licenses = %w(LGPL-2.1+)
+  s.licenses = %w(LGPL-2.1-or-later)
 end
-- 
2.43.0

[PATCH 2/2] rack 3.x compatibility

[Stefan Sundin]

Rack::Utils::HeaderHash will be removed in rack 3.1 so these changes mostly
address that. The initializer in Rack::Headers inherits from Hash, so switching
to the ::[] class method to achieve the same result.

The test test_broken_header_response was removed as Rack::Headers now throws
an error in that case. I don't know if that necessarily is a good test case?

Since the rackup functionality was split out to its own gem, the test case
test_lint_error_wrapper now only works with rack 3.x.

Other than that exception, all tests are passing with rack 1.6.13, 2.2.8,
and 3.0.8.
---
 clogger.gemspec              |  3 ++-
 ext/clogger_ext/clogger.c    | 23 ++++++++++--------
 lib/clogger/pure.rb          | 12 ++++++++--
 test/test_clogger.rb         | 45 +++++++++++++++++++-----------------
 test/test_clogger_to_path.rb | 14 ++++++-----
 5 files changed, 58 insertions(+), 39 deletions(-)

diff --git a/clogger.gemspec b/clogger.gemspec
index ab479c5..7a08dc5 100644
--- a/clogger.gemspec
+++ b/clogger.gemspec
@@ -16,8 +16,9 @@ Gem::Specification.new do |s|
   s.test_files = %w(test/test_clogger.rb test/test_clogger_to_path.rb)
 
   # HeaderHash wasn't case-insensitive in old versions
-  s.add_dependency(%q<rack>, ['>= 1.0', '< 3.0'])
+  s.add_dependency(%q<rack>, ['>= 1.0', '< 4.0'])
   s.add_development_dependency('test-unit', '~> 3.0')
+  s.add_development_dependency('rackup', '~> 2.0')
   s.extensions = %w(ext/clogger_ext/extconf.rb)
   s.licenses = %w(LGPL-2.1-or-later)
 end
diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c
index 23aea39..9ccafb5 100644
--- a/ext/clogger_ext/clogger.c
+++ b/ext/clogger_ext/clogger.c
@@ -136,7 +136,7 @@ static ID to_path_id;
 static ID respond_to_id;
 static VALUE cClogger;
 static VALUE mFormat;
-static VALUE cHeaderHash;
+static VALUE cRackHeaders;
 
 /* common hash lookup keys */
 static VALUE g_HTTP_X_FORWARDED_FOR;
@@ -628,7 +628,7 @@ static void append_response(struct clogger *c, VALUE key)
 {
 	VALUE v;
 
-	assert(rb_obj_is_kind_of(c->headers, cHeaderHash) && "not HeaderHash");
+	assert(rb_obj_is_kind_of(c->headers, cRackHeaders) && "not Rack::Headers");
 
 	v = rb_funcall(c->headers, sq_brace_id, 1, key);
 	v = NIL_P(v) ? g_dash : byte_xs(v);
@@ -900,10 +900,9 @@ static VALUE ccall(struct clogger *c, VALUE env)
 
 		rv = rb_ary_dup(rv);
 		if (c->need_resp &&
-                    ! rb_obj_is_kind_of(c->headers, cHeaderHash)) {
-			c->headers = rb_funcall(cHeaderHash, new_id, 1,
-			                        c->headers);
-			rb_ary_store(rv, 1, c->headers);
+                    ! rb_obj_is_kind_of(c->headers, cRackHeaders)) {
+			c->headers = rb_funcall(cRackHeaders, sq_brace_id,
+			                        1, c->headers);
 		}
 	} else {
 		VALUE tmp = rb_inspect(rv);
@@ -1114,9 +1113,15 @@ void Init_clogger_ext(void)
 	CONST_GLOBAL_STR2(rack_request_cookie_hash, "rack.request.cookie_hash");
 
 	tmp = rb_const_get(rb_cObject, rb_intern("Rack"));
-	tmp = rb_const_get(tmp, rb_intern("Utils"));
-	cHeaderHash = rb_const_get(tmp, rb_intern("HeaderHash"));
-	rb_ary_push(mark_ary, cHeaderHash);
+	if (rb_const_defined(tmp, rb_intern("Headers"))) {
+		// Rack >= 3.0
+		cRackHeaders = rb_const_get(tmp, rb_intern("Headers"));
+	} else {
+		// Rack < 3.0
+		tmp = rb_const_get(tmp, rb_intern("Utils"));
+		cRackHeaders = rb_const_get(tmp, rb_intern("HeaderHash"));
+	}
+	rb_ary_push(mark_ary, cRackHeaders);
 
 	rb_obj_freeze(mark_ary);
 }
diff --git a/lib/clogger/pure.rb b/lib/clogger/pure.rb
index 4b38e90..1ddd7f2 100644
--- a/lib/clogger/pure.rb
+++ b/lib/clogger/pure.rb
@@ -8,9 +8,17 @@ class Clogger
   attr_accessor :env, :status, :headers, :body
   attr_writer :body_bytes_sent, :start
 
+  RackHeaders = if Object.const_defined?("Rack::Headers")
+    # Rack >= 3.0
+    Rack::Headers
+  else
+    # Rack < 3.0
+    Rack::Utils::HeaderHash
+  end
+
   def initialize(app, opts = {})
     # trigger autoload to avoid thread-safety issues later on
-    Rack::Utils::HeaderHash
+    RackHeaders
 
     @app = app
     @logger = opts[:logger]
@@ -35,7 +43,7 @@ class Clogger
       raise TypeError, "app response not a 3 element Array: #{resp.inspect}"
     end
     status, headers, body = resp
-    headers = Rack::Utils::HeaderHash.new(headers) if @need_resp
+    headers = RackHeaders[headers] if @need_resp
     if @wrap_body
       @reentrant = env['rack.multithread'] if @reentrant.nil?
       wbody = @reentrant ? self.dup : self
diff --git a/test/test_clogger.rb b/test/test_clogger.rb
index 1dee652..bbb5791 100644
--- a/test/test_clogger.rb
+++ b/test/test_clogger.rb
@@ -10,6 +10,14 @@ require "rack"
 
 require "clogger"
 
+RackHeaders = if Object.const_defined?("Rack::Headers")
+  # Rack >= 3.0
+  Rack::Headers
+else
+  # Rack < 3.0
+  Rack::Utils::HeaderHash
+end
+
 # used to test subclasses
 class FooString < String
 end
@@ -22,6 +30,7 @@ class TestClogger < Test::Unit::TestCase
     @nginx_fmt = "%d/%b/%Y:%H:%M:%S %z"
     @req = {
       "REQUEST_METHOD" => "GET",
+      "SERVER_PROTOCOL" => "HTTP/1.0",
       "HTTP_VERSION" => "HTTP/1.0",
       "HTTP_USER_AGENT" => 'echo and socat \o/',
       "PATH_INFO" => "/hello",
@@ -85,13 +94,13 @@ class TestClogger < Test::Unit::TestCase
   def test_clen_stringio
     start = DateTime.now - 1
     str = StringIO.new
-    app = lambda { |env| [ 301, {'Content-Length' => '5'}, ['abcde'] ] }
+    app = lambda { |env| [ 301, {'content-length' => '5'}, ['abcde'] ] }
     format = Common.dup
     assert format.gsub!(/response_length/, 'sent_http_content_length')
     cl = Clogger.new(app, :logger => str, :format => format)
     status, headers, body = cl.call(@req)
     assert_equal(301, status)
-    assert_equal({'Content-Length' => '5'}, headers)
+    assert_equal({'content-length' => '5'}, headers)
     body.each { |part| assert_equal('abcde', part) }
     str = str.string
     r = %r{\Ahome - - \[[^\]]+\] "GET /hello\?goodbye=true HTTP/1.0" 301 5\n\z}
@@ -253,7 +262,7 @@ class TestClogger < Test::Unit::TestCase
   def test_rack_1_0
     start = DateTime.now - 1
     str = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'0'}, %w(a b c)] }
+    app = lambda { |env| [ 200, {'content-length'=>'0'}, %w(a b c)] }
     cl = Clogger.new(app, :logger => str, :format => Rack_1_0)
     status, headers, body = cl.call(@req)
     tmp = []
@@ -344,7 +353,7 @@ class TestClogger < Test::Unit::TestCase
   def test_combined
     start = DateTime.now - 1
     str = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'3'}, %w(a b c)] }
+    app = lambda { |env| [ 200, {'content-length'=>'3'}, %w(a b c)] }
     cl = Clogger.new(app, :logger => str, :format => Combined)
     status, headers, body = cl.call(@req)
     tmp = []
@@ -365,7 +374,7 @@ class TestClogger < Test::Unit::TestCase
 
   def test_rack_errors_fallback
     err = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'3'}, %w(a b c)] }
+    app = lambda { |env| [ 200, {'content-length'=>'3'}, %w(a b c)] }
     cl = Clogger.new(app, :format => '$pid')
     req = @req.merge('rack.errors' => err)
     status, headers, body = cl.call(req)
@@ -374,7 +383,7 @@ class TestClogger < Test::Unit::TestCase
 
   def test_body_close
     s_body = StringIO.new(%w(a b c).join("\n"))
-    app = lambda { |env| [ 200, {'Content-Length'=>'5'}, s_body] }
+    app = lambda { |env| [ 200, {'content-length'=>'5'}, s_body] }
     cl = Clogger.new(app, :logger => [], :format => '$pid')
     status, headers, body = cl.call(@req)
     assert ! s_body.closed?
@@ -384,7 +393,7 @@ class TestClogger < Test::Unit::TestCase
 
   def test_escape
     str = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'5'}, [] ] }
+    app = lambda { |env| [ 200, {'content-length'=>'5'}, [] ] }
     cl = Clogger.new(app,
       :logger => str,
       :format => '$http_user_agent "$request"')
@@ -399,12 +408,12 @@ class TestClogger < Test::Unit::TestCase
   end
 
   # rack allows repeated headers with "\n":
-  # { 'Set-Cookie' => "a\nb" } =>
-  #   Set-Cookie: a
-  #   Set-Cookie: b
+  # { 'set-cookie' => "a\nb" } =>
+  #   set-cookie: a
+  #   set-cookie: b
   def test_escape_header_newlines
     str = StringIO.new
-    app = lambda { |env| [302, { 'Set-Cookie' => "a\nb" }, [] ] }
+    app = lambda { |env| [302, { 'set-cookie' => "a\nb" }, [] ] }
     cl = Clogger.new(app, :logger => str, :format => '$sent_http_set_cookie')
     cl.call(@req)
     assert_equal "a\\x0Ab\n", str.string
@@ -459,16 +468,9 @@ class TestClogger < Test::Unit::TestCase
     assert_match %r{#{e}$}m, str
   end
 
-  def test_broken_header_response
-    str = StringIO.new
-    app = lambda { |env| [302, [ %w(a) ], []] }
-    cl = Clogger.new(app, :logger => str, :format => '$sent_http_set_cookie')
-    assert_nothing_raised { cl.call(@req) }
-  end
-
   def test_subclass_hash
     str = StringIO.new
-    req = Rack::Utils::HeaderHash.new(@req)
+    req = RackHeaders[@req]
     app = lambda { |env| [302, [ %w(a) ], []] }
     cl = Clogger.new(app, :logger => str, :format => Rack_1_0)
     assert_nothing_raised { cl.call(req).last.each {}.close }
@@ -877,14 +879,15 @@ class TestClogger < Test::Unit::TestCase
   end
 
   def test_lint_error_wrapper
-    require 'rack/lobster'
+    # This test only works on Rack >= 3.0
+    require 'rackup/lobster'
     @req["SERVER_NAME"] = "FOO"
     @req["SERVER_PORT"] = "666"
     @req["rack.version"] = [1,1]
     @req["rack.multithread"] = true
     @req["rack.multiprocess"] = true
     @req["rack.run_once"] = false
-    app = Rack::ContentLength.new(Rack::ContentType.new(Rack::Lobster.new))
+    app = Rack::ContentLength.new(Rack::ContentType.new(Rackup::Lobster.new))
     cl = Clogger.new(app, :format => :Combined)
     @req["rack.errors"] = err = StringIO.new
     status, headers, body = r = Rack::Lint.new(cl).call(@req)
diff --git a/test/test_clogger_to_path.rb b/test/test_clogger_to_path.rb
index f74b991..4cc1738 100644
--- a/test/test_clogger_to_path.rb
+++ b/test/test_clogger_to_path.rb
@@ -3,6 +3,7 @@ $stderr.sync = $stdout.sync = true
 require "test/unit"
 require "date"
 require "stringio"
+require "tempfile"
 require "rack"
 require "clogger"
 
@@ -26,6 +27,7 @@ class TestCloggerToPath < Test::Unit::TestCase
     @req = {
       "REQUEST_METHOD" => "GET",
       "HTTP_VERSION" => "HTTP/1.0",
+      "SERVER_PROTOCOL" => "HTTP/1.0",
       "HTTP_USER_AGENT" => 'echo and socat \o/',
       "PATH_INFO" => "/",
       "QUERY_STRING" => "",
@@ -50,8 +52,8 @@ class TestCloggerToPath < Test::Unit::TestCase
     app = Rack::Builder.new do
       tmp.syswrite(' ' * 365)
       h = {
-        'Content-Length' => '0',
-        'Content-Type' => 'text/plain',
+        'content-length' => '0',
+        'content-type' => 'text/plain',
       }
       use Clogger,
         :logger => logger,
@@ -82,8 +84,8 @@ class TestCloggerToPath < Test::Unit::TestCase
     app = Rack::Builder.new do
       tmp.syswrite(' ' * 365)
       h = {
-        'Content-Length' => '0',
-        'Content-Type' => 'text/plain',
+        'content-length' => '0',
+        'content-type' => 'text/plain',
       }
       use Clogger,
         :logger => logger,
@@ -106,8 +108,8 @@ class TestCloggerToPath < Test::Unit::TestCase
     logger = StringIO.new
     app = Rack::Builder.new do
       h = {
-        'Content-Length' => '3',
-        'Content-Type' => 'text/plain',
+        'content-length' => '3',
+        'content-type' => 'text/plain',
       }
       use Clogger,
         :logger => logger,
-- 
2.43.0

Re: [PATCH 2/2] rack 3.x compatibility

[Eric Wong]

Stefan Sundin <git@stefansundin.com> wrote:
> Rack::Utils::HeaderHash will be removed in rack 3.1 so these changes mostly
> address that. The initializer in Rack::Headers inherits from Hash, so switching
> to the ::[] class method to achieve the same result.

OK.  Fwiw, I don't do much Ruby nowadays and will be keeping my
legacy projects Rack 2 for the forseeable future.

Patch 1 for the LGPL wording change is fine and I was able to
apply your patches without problems.

> The test test_broken_header_response was removed as Rack::Headers now throws
> an error in that case. I don't know if that necessarily is a good test case?

It's not a great test case, but I prefer we test malformed
responses in some way (more inline..)

> Since the rackup functionality was split out to its own gem, the test case
> test_lint_error_wrapper now only works with rack 3.x.

OK (more inline...)

> Other than that exception, all tests are passing with rack 1.6.13, 2.2.8,
> and 3.0.8.

Good to know.

> --- a/clogger.gemspec
> +++ b/clogger.gemspec
> @@ -16,8 +16,9 @@ Gem::Specification.new do |s|
>    s.test_files = %w(test/test_clogger.rb test/test_clogger_to_path.rb)
>  
>    # HeaderHash wasn't case-insensitive in old versions
> -  s.add_dependency(%q<rack>, ['>= 1.0', '< 3.0'])
> +  s.add_dependency(%q<rack>, ['>= 1.0', '< 4.0'])
>    s.add_development_dependency('test-unit', '~> 3.0')
> +  s.add_development_dependency('rackup', '~> 2.0')

I think rackup can be optional and we can rescue NameError
(below) if missing.

>  static VALUE g_HTTP_X_FORWARDED_FOR;
> @@ -628,7 +628,7 @@ static void append_response(struct clogger *c, VALUE key)
>  {
>  	VALUE v;
>  
> -	assert(rb_obj_is_kind_of(c->headers, cHeaderHash) && "not HeaderHash");
> +	assert(rb_obj_is_kind_of(c->headers, cRackHeaders) && "not Rack::Headers");

Please wrap at <80 columns (I need gigantic fonts to see) Thanks.

> +  RackHeaders = if Object.const_defined?("Rack::Headers")
> +    # Rack >= 3.0
> +    Rack::Headers
> +  else
> +    # Rack < 3.0
> +    Rack::Utils::HeaderHash
> +  end
> +
>    def initialize(app, opts = {})
>      # trigger autoload to avoid thread-safety issues later on
> -    Rack::Utils::HeaderHash
> +    RackHeaders

Minor: the explicit autoload trigger is no longer needed since the
above if/else already triggered it.

<snip>

> @@ -399,12 +408,12 @@ class TestClogger < Test::Unit::TestCase
>    end
>  
>    # rack allows repeated headers with "\n":
> -  # { 'Set-Cookie' => "a\nb" } =>
> -  #   Set-Cookie: a
> -  #   Set-Cookie: b
> +  # { 'set-cookie' => "a\nb" } =>
> +  #   set-cookie: a
> +  #   set-cookie: b
>    def test_escape_header_newlines
>      str = StringIO.new
> -    app = lambda { |env| [302, { 'Set-Cookie' => "a\nb" }, [] ] }
> +    app = lambda { |env| [302, { 'set-cookie' => "a\nb" }, [] ] }

Important:

Rack 3 uses arrays instead of "\n" to denote multi-values.
So I think the test (and clogger itself) need to to account
for array values (but continue to support Rack 2 "\n"-delimited
multi-values)

> -  def test_broken_header_response
> -    str = StringIO.new
> -    app = lambda { |env| [302, [ %w(a) ], []] }
> -    cl = Clogger.new(app, :logger => str, :format => '$sent_http_set_cookie')
> -    assert_nothing_raised { cl.call(@req) }
> -  end

It's fine if the broken header raises w/ Rack 3.
I mainly want to make sure a malformed response doesn't
trigger a segfault or memory corruption on our end.
So whether it raises or silently ignores the invalid response is
fine.

> @@ -877,14 +879,15 @@ class TestClogger < Test::Unit::TestCase
>    end
>  
>    def test_lint_error_wrapper
> -    require 'rack/lobster'
> +    # This test only works on Rack >= 3.0
> +    require 'rackup/lobster'

I prefer we rescue NameError here and skip tests if rackup or
lobster is missing.  It's not important to run the odd tests if
we can save some bandwidth and storage.

I also noticed some problems with Ruby 3.3, will post followups
in a bit.

Thanks.

v2

[Stefan Sundin]

Thank you for the review. I think I have addressed most, if not all, of the issues.

Still learning email based patch workflow, so please feel free to let me know if I'm doing anything wrong.

Regards, Stefan

[PATCH v2 1/2] update deprecated license identifier

[Stefan Sundin]

---
 clogger.gemspec | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/clogger.gemspec b/clogger.gemspec
index d461383..ab479c5 100644
--- a/clogger.gemspec
+++ b/clogger.gemspec
@@ -19,5 +19,5 @@ Gem::Specification.new do |s|
   s.add_dependency(%q<rack>, ['>= 1.0', '< 3.0'])
   s.add_development_dependency('test-unit', '~> 3.0')
   s.extensions = %w(ext/clogger_ext/extconf.rb)
-  s.licenses = %w(LGPL-2.1+)
+  s.licenses = %w(LGPL-2.1-or-later)
 end
-- 
2.43.0

[PATCH v2 2/2] rack 3.x compatibility

[Stefan Sundin]

Rack::Utils::HeaderHash will be removed in rack 3.1 so these changes mostly
address that. The initializer in Rack::Headers inherits from Hash, so
switching to the ::[] class method to achieve the same result.
---
 clogger.gemspec              |  3 +-
 ext/clogger_ext/clogger.c    | 28 +++++++++++------
 lib/clogger/pure.rb          | 14 +++++++--
 test/test_clogger.rb         | 61 +++++++++++++++++++++++++-----------
 test/test_clogger_to_path.rb | 14 +++++----
 5 files changed, 83 insertions(+), 37 deletions(-)

diff --git a/clogger.gemspec b/clogger.gemspec
index ab479c5..7a08dc5 100644
--- a/clogger.gemspec
+++ b/clogger.gemspec
@@ -16,8 +16,9 @@ Gem::Specification.new do |s|
   s.test_files = %w(test/test_clogger.rb test/test_clogger_to_path.rb)
 
   # HeaderHash wasn't case-insensitive in old versions
-  s.add_dependency(%q<rack>, ['>= 1.0', '< 3.0'])
+  s.add_dependency(%q<rack>, ['>= 1.0', '< 4.0'])
   s.add_development_dependency('test-unit', '~> 3.0')
+  s.add_development_dependency('rackup', '~> 2.0')
   s.extensions = %w(ext/clogger_ext/extconf.rb)
   s.licenses = %w(LGPL-2.1-or-later)
 end
diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c
index 23aea39..1993ae8 100644
--- a/ext/clogger_ext/clogger.c
+++ b/ext/clogger_ext/clogger.c
@@ -136,7 +136,7 @@ static ID to_path_id;
 static ID respond_to_id;
 static VALUE cClogger;
 static VALUE mFormat;
-static VALUE cHeaderHash;
+static VALUE cRackHeaders;
 
 /* common hash lookup keys */
 static VALUE g_HTTP_X_FORWARDED_FOR;
@@ -172,6 +172,10 @@ static VALUE byte_xs(VALUE obj)
 {
 	static const char esc[] = "0123456789ABCDEF";
 	unsigned char *new_ptr;
+	if (rb_obj_is_kind_of(obj, rb_cArray)) {
+		// Rack 3
+		obj = rb_ary_join(obj, rb_str_new2("\n"));
+	}
 	VALUE from = rb_obj_as_string(obj);
 	const unsigned char *ptr = (const unsigned char *)RSTRING_PTR(from);
 	long len = RSTRING_LEN(from);
@@ -628,7 +632,8 @@ static void append_response(struct clogger *c, VALUE key)
 {
 	VALUE v;
 
-	assert(rb_obj_is_kind_of(c->headers, cHeaderHash) && "not HeaderHash");
+	assert(rb_obj_is_kind_of(c->headers, cRackHeaders)
+		&& "not Rack::Headers");
 
 	v = rb_funcall(c->headers, sq_brace_id, 1, key);
 	v = NIL_P(v) ? g_dash : byte_xs(v);
@@ -900,10 +905,9 @@ static VALUE ccall(struct clogger *c, VALUE env)
 
 		rv = rb_ary_dup(rv);
 		if (c->need_resp &&
-                    ! rb_obj_is_kind_of(c->headers, cHeaderHash)) {
-			c->headers = rb_funcall(cHeaderHash, new_id, 1,
-			                        c->headers);
-			rb_ary_store(rv, 1, c->headers);
+                    ! rb_obj_is_kind_of(c->headers, cRackHeaders)) {
+			c->headers = rb_funcall(cRackHeaders, sq_brace_id,
+			                        1, c->headers);
 		}
 	} else {
 		VALUE tmp = rb_inspect(rv);
@@ -1114,9 +1118,15 @@ void Init_clogger_ext(void)
 	CONST_GLOBAL_STR2(rack_request_cookie_hash, "rack.request.cookie_hash");
 
 	tmp = rb_const_get(rb_cObject, rb_intern("Rack"));
-	tmp = rb_const_get(tmp, rb_intern("Utils"));
-	cHeaderHash = rb_const_get(tmp, rb_intern("HeaderHash"));
-	rb_ary_push(mark_ary, cHeaderHash);
+	if (rb_const_defined(tmp, rb_intern("Headers"))) {
+		// Rack >= 3.0
+		cRackHeaders = rb_const_get(tmp, rb_intern("Headers"));
+	} else {
+		// Rack < 3.0
+		tmp = rb_const_get(tmp, rb_intern("Utils"));
+		cRackHeaders = rb_const_get(tmp, rb_intern("HeaderHash"));
+	}
+	rb_ary_push(mark_ary, cRackHeaders);
 
 	rb_obj_freeze(mark_ary);
 }
diff --git a/lib/clogger/pure.rb b/lib/clogger/pure.rb
index 4b38e90..156f11e 100644
--- a/lib/clogger/pure.rb
+++ b/lib/clogger/pure.rb
@@ -8,10 +8,15 @@ class Clogger
   attr_accessor :env, :status, :headers, :body
   attr_writer :body_bytes_sent, :start
 
-  def initialize(app, opts = {})
-    # trigger autoload to avoid thread-safety issues later on
+  RackHeaders = if Object.const_defined?("Rack::Headers")
+    # Rack >= 3.0
+    Rack::Headers
+  else
+    # Rack < 3.0
     Rack::Utils::HeaderHash
+  end
 
+  def initialize(app, opts = {})
     @app = app
     @logger = opts[:logger]
     path = opts[:path]
@@ -35,7 +40,7 @@ class Clogger
       raise TypeError, "app response not a 3 element Array: #{resp.inspect}"
     end
     status, headers, body = resp
-    headers = Rack::Utils::HeaderHash.new(headers) if @need_resp
+    headers = RackHeaders[headers] if @need_resp
     if @wrap_body
       @reentrant = env['rack.multithread'] if @reentrant.nil?
       wbody = @reentrant ? self.dup : self
@@ -91,6 +96,9 @@ private
 
   def byte_xs(s)
     s = s.dup
+    if s.is_a?(Array)
+      s = s.join("\n")
+    end
     s.force_encoding(Encoding::BINARY) if defined?(Encoding::BINARY)
     s.gsub!(/(['"\x00-\x1f\x7f-\xff])/) do |x|
       "\\x#{$1.unpack('H2').first.upcase}"
diff --git a/test/test_clogger.rb b/test/test_clogger.rb
index 1dee652..5646025 100644
--- a/test/test_clogger.rb
+++ b/test/test_clogger.rb
@@ -10,6 +10,16 @@ require "rack"
 
 require "clogger"
 
+IS_RACK3 = Gem::Version.new(Rack.release) >= Gem::Version.new('3.0.0')
+
+RackHeaders = if Object.const_defined?("Rack::Headers")
+  # Rack >= 3.0
+  Rack::Headers
+else
+  # Rack < 3.0
+  Rack::Utils::HeaderHash
+end
+
 # used to test subclasses
 class FooString < String
 end
@@ -22,6 +32,7 @@ class TestClogger < Test::Unit::TestCase
     @nginx_fmt = "%d/%b/%Y:%H:%M:%S %z"
     @req = {
       "REQUEST_METHOD" => "GET",
+      "SERVER_PROTOCOL" => "HTTP/1.0",
       "HTTP_VERSION" => "HTTP/1.0",
       "HTTP_USER_AGENT" => 'echo and socat \o/',
       "PATH_INFO" => "/hello",
@@ -85,13 +96,13 @@ class TestClogger < Test::Unit::TestCase
   def test_clen_stringio
     start = DateTime.now - 1
     str = StringIO.new
-    app = lambda { |env| [ 301, {'Content-Length' => '5'}, ['abcde'] ] }
+    app = lambda { |env| [ 301, {'content-length' => '5'}, ['abcde'] ] }
     format = Common.dup
     assert format.gsub!(/response_length/, 'sent_http_content_length')
     cl = Clogger.new(app, :logger => str, :format => format)
     status, headers, body = cl.call(@req)
     assert_equal(301, status)
-    assert_equal({'Content-Length' => '5'}, headers)
+    assert_equal({'content-length' => '5'}, headers)
     body.each { |part| assert_equal('abcde', part) }
     str = str.string
     r = %r{\Ahome - - \[[^\]]+\] "GET /hello\?goodbye=true HTTP/1.0" 301 5\n\z}
@@ -253,7 +264,7 @@ class TestClogger < Test::Unit::TestCase
   def test_rack_1_0
     start = DateTime.now - 1
     str = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'0'}, %w(a b c)] }
+    app = lambda { |env| [ 200, {'content-length'=>'0'}, %w(a b c)] }
     cl = Clogger.new(app, :logger => str, :format => Rack_1_0)
     status, headers, body = cl.call(@req)
     tmp = []
@@ -344,7 +355,7 @@ class TestClogger < Test::Unit::TestCase
   def test_combined
     start = DateTime.now - 1
     str = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'3'}, %w(a b c)] }
+    app = lambda { |env| [ 200, {'content-length'=>'3'}, %w(a b c)] }
     cl = Clogger.new(app, :logger => str, :format => Combined)
     status, headers, body = cl.call(@req)
     tmp = []
@@ -365,7 +376,7 @@ class TestClogger < Test::Unit::TestCase
 
   def test_rack_errors_fallback
     err = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'3'}, %w(a b c)] }
+    app = lambda { |env| [ 200, {'content-length'=>'3'}, %w(a b c)] }
     cl = Clogger.new(app, :format => '$pid')
     req = @req.merge('rack.errors' => err)
     status, headers, body = cl.call(req)
@@ -374,7 +385,7 @@ class TestClogger < Test::Unit::TestCase
 
   def test_body_close
     s_body = StringIO.new(%w(a b c).join("\n"))
-    app = lambda { |env| [ 200, {'Content-Length'=>'5'}, s_body] }
+    app = lambda { |env| [ 200, {'content-length'=>'5'}, s_body] }
     cl = Clogger.new(app, :logger => [], :format => '$pid')
     status, headers, body = cl.call(@req)
     assert ! s_body.closed?
@@ -384,7 +395,7 @@ class TestClogger < Test::Unit::TestCase
 
   def test_escape
     str = StringIO.new
-    app = lambda { |env| [ 200, {'Content-Length'=>'5'}, [] ] }
+    app = lambda { |env| [ 200, {'content-length'=>'5'}, [] ] }
     cl = Clogger.new(app,
       :logger => str,
       :format => '$http_user_agent "$request"')
@@ -398,13 +409,21 @@ class TestClogger < Test::Unit::TestCase
     assert_equal expect, str.string
   end
 
-  # rack allows repeated headers with "\n":
-  # { 'Set-Cookie' => "a\nb" } =>
-  #   Set-Cookie: a
-  #   Set-Cookie: b
-  def test_escape_header_newlines
-    str = StringIO.new
-    app = lambda { |env| [302, { 'Set-Cookie' => "a\nb" }, [] ] }
+  # rack >= 3 allows repeated headers with array:
+  # { 'set-cookie' => ["a","b"] } =>
+  #   set-cookie: a
+  #   set-cookie: b
+  # rack < 3 allows repeated headers with "\n":
+  # { 'set-cookie' => "a\nb" } =>
+  #   set-cookie: a
+  #   set-cookie: b
+  def test_multiheader
+    str = StringIO.new
+    if IS_RACK3 then
+      app = lambda { |env| [302, { 'set-cookie' => ["a","b"] }, [] ] }
+    else
+      app = lambda { |env| [302, { 'set-cookie' => "a\nb" }, [] ] }
+    end
     cl = Clogger.new(app, :logger => str, :format => '$sent_http_set_cookie')
     cl.call(@req)
     assert_equal "a\\x0Ab\n", str.string
@@ -463,12 +482,16 @@ class TestClogger < Test::Unit::TestCase
     str = StringIO.new
     app = lambda { |env| [302, [ %w(a) ], []] }
     cl = Clogger.new(app, :logger => str, :format => '$sent_http_set_cookie')
-    assert_nothing_raised { cl.call(@req) }
+    if IS_RACK3 then
+      assert_raise(ArgumentError) { cl.call(@req) }
+    else
+      assert_nothing_raised { cl.call(@req) }
+    end
   end
 
   def test_subclass_hash
     str = StringIO.new
-    req = Rack::Utils::HeaderHash.new(@req)
+    req = RackHeaders[@req]
     app = lambda { |env| [302, [ %w(a) ], []] }
     cl = Clogger.new(app, :logger => str, :format => Rack_1_0)
     assert_nothing_raised { cl.call(req).last.each {}.close }
@@ -877,19 +900,21 @@ class TestClogger < Test::Unit::TestCase
   end
 
   def test_lint_error_wrapper
-    require 'rack/lobster'
+    require 'rackup/lobster'
     @req["SERVER_NAME"] = "FOO"
     @req["SERVER_PORT"] = "666"
     @req["rack.version"] = [1,1]
     @req["rack.multithread"] = true
     @req["rack.multiprocess"] = true
     @req["rack.run_once"] = false
-    app = Rack::ContentLength.new(Rack::ContentType.new(Rack::Lobster.new))
+    app = Rack::ContentLength.new(Rack::ContentType.new(Rackup::Lobster.new))
     cl = Clogger.new(app, :format => :Combined)
     @req["rack.errors"] = err = StringIO.new
     status, headers, body = r = Rack::Lint.new(cl).call(@req)
     body.each { |x| assert_kind_of String, x.to_str }
     body.close # might raise here
     assert_match(%r{GET /hello}, err.string)
+  rescue LoadError, Gem::ConflictError
+    # This test only works on Rack >= 3.0
   end
 end
diff --git a/test/test_clogger_to_path.rb b/test/test_clogger_to_path.rb
index f74b991..4cc1738 100644
--- a/test/test_clogger_to_path.rb
+++ b/test/test_clogger_to_path.rb
@@ -3,6 +3,7 @@ $stderr.sync = $stdout.sync = true
 require "test/unit"
 require "date"
 require "stringio"
+require "tempfile"
 require "rack"
 require "clogger"
 
@@ -26,6 +27,7 @@ class TestCloggerToPath < Test::Unit::TestCase
     @req = {
       "REQUEST_METHOD" => "GET",
       "HTTP_VERSION" => "HTTP/1.0",
+      "SERVER_PROTOCOL" => "HTTP/1.0",
       "HTTP_USER_AGENT" => 'echo and socat \o/',
       "PATH_INFO" => "/",
       "QUERY_STRING" => "",
@@ -50,8 +52,8 @@ class TestCloggerToPath < Test::Unit::TestCase
     app = Rack::Builder.new do
       tmp.syswrite(' ' * 365)
       h = {
-        'Content-Length' => '0',
-        'Content-Type' => 'text/plain',
+        'content-length' => '0',
+        'content-type' => 'text/plain',
       }
       use Clogger,
         :logger => logger,
@@ -82,8 +84,8 @@ class TestCloggerToPath < Test::Unit::TestCase
     app = Rack::Builder.new do
       tmp.syswrite(' ' * 365)
       h = {
-        'Content-Length' => '0',
-        'Content-Type' => 'text/plain',
+        'content-length' => '0',
+        'content-type' => 'text/plain',
       }
       use Clogger,
         :logger => logger,
@@ -106,8 +108,8 @@ class TestCloggerToPath < Test::Unit::TestCase
     logger = StringIO.new
     app = Rack::Builder.new do
       h = {
-        'Content-Length' => '3',
-        'Content-Type' => 'text/plain',
+        'content-length' => '3',
+        'content-type' => 'text/plain',
       }
       use Clogger,
         :logger => logger,
-- 
2.43.0

Re: v2

[Eric Wong]

Stefan Sundin <git@stefansundin.com> wrote:
> Thank you for the review. I think I have addressed most, if
> not all, of the issues.

Thanks, pushed to clogger.git as
commit 6b0768f8791c5d6dd2e8a5aea0da76d46549477e
and 2991f00afa4e445214f3b997bb37cb746c01cd2d

I've also pushed out TypedData memory leak workaround;
might try to fix some deprecation warnings while testing
some other stuff but personal things and storms + water leaks
keep getting in the way :<

> Still learning email based patch workflow, so please feel free
> to let me know if I'm doing anything wrong.

Seems fine :>  If you have documentation improvements to git.git,
I'm sure folks at git@vger.kernel.org would be happy to look at
them :>

[PATCH] test_broken_header_response: allow NoMethodError w/ rack 2

[Eric Wong]

Just a minor problem I noticed on my other machine since I'm way
behind on upgrades in `production'...  Also 2 more minor patches
being worked on (will post on https://yhbt.net/clogger-public/
w/o Cc in a bit)

-----8<------
Subject: [PATCH] test_broken_header_response: allow NoMethodError w/ rack 2

Debian 11 (oldstable) packages rack 2.1.4 which fails with
NoMethodError when passed an invalid response header.  rack
2.2.6.4 on Debian 12 (stable) doesn't raise, however.
---
 test/test_clogger.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/test/test_clogger.rb b/test/test_clogger.rb
index 5646025..955946b 100644
--- a/test/test_clogger.rb
+++ b/test/test_clogger.rb
@@ -485,7 +485,10 @@ def test_broken_header_response
     if IS_RACK3 then
       assert_raise(ArgumentError) { cl.call(@req) }
     else
-      assert_nothing_raised { cl.call(@req) }
+      begin
+        cl.call(@req)
+      rescue NoMethodError # rack 2.1.4 raises, but not 2.2.6.4
+      end
     end
   end