[PATCH] Update respond_to? calls for second argument.

[PATCH] Update respond_to? calls for second argument.

[Pat Allan]

Rack (since v2) has started explicitly listing the second (optional) argument for respond_to?, which matches the underlying Ruby spec. This patch fixes the calls in both C and Ruby approaches.

However, rb_respond_to only accepts a single argument - differing from the Ruby side of things - so perhaps this patch isn't quite perfect (and my C skills are very limited, so the whole thing could use a review).
---
 ext/clogger_ext/clogger.c | 8 ++++++--
 lib/clogger/pure.rb       | 4 ++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c
index 481dd61..622c98c 100644
--- a/ext/clogger_ext/clogger.c
+++ b/ext/clogger_ext/clogger.c
@@ -963,8 +963,12 @@ static VALUE clogger_init_copy(VALUE clone, VALUE orig)
  * used to delegate +:to_path+ checks for Rack webservers that optimize
  * static file serving
  */
-static VALUE respond_to(VALUE self, VALUE method)
+static VALUE respond_to(int argc, VALUE *argv, VALUE self)
 {
+	VALUE method, include_all;
+	rb_scan_args(argc, argv, "11", &method, &include_all);
+	if (NIL_P(include_all)) include_all = Qfalse;
+
 	struct clogger *c = clogger_get(self);
 	ID id = rb_to_id(method);
 
@@ -1044,7 +1048,7 @@ void Init_clogger_ext(void)
 	rb_define_method(cClogger, "wrap_body?", clogger_wrap_body, 0);
 	rb_define_method(cClogger, "reentrant?", clogger_reentrant, 0);
 	rb_define_method(cClogger, "to_path", to_path, 0);
-	rb_define_method(cClogger, "respond_to?", respond_to, 1);
+	rb_define_method(cClogger, "respond_to?", respond_to, -1);
 	rb_define_method(cClogger, "body", body, 0);
 	CONST_GLOBAL_STR(REMOTE_ADDR);
 	CONST_GLOBAL_STR(HTTP_X_FORWARDED_FOR);
diff --git a/lib/clogger/pure.rb b/lib/clogger/pure.rb
index 77f81b4..fddfe79 100644
--- a/lib/clogger/pure.rb
+++ b/lib/clogger/pure.rb
@@ -77,8 +77,8 @@ class Clogger
     @logger.respond_to?(:fileno) ? @logger.fileno : nil
   end
 
-  def respond_to?(m)
-    :close == m.to_sym || @body.respond_to?(m)
+  def respond_to?(method, include_all=false)
+    :close == method.to_sym || @body.respond_to?(method, include_all)
   end
 
   def to_path
-- 
Pat

Re: [PATCH] Update respond_to? calls for second argument.

[Eric Wong]

Pat Allan <pat@freelancing-gods.com> wrote:
> Rack (since v2) has started explicitly listing the second
> (optional) argument for respond_to?, which matches the
> underlying Ruby spec. This patch fixes the calls in both C and
> Ruby approaches.

Thanks for noticing this!

> However, rb_respond_to only accepts a single argument -
> differing from the Ruby side of things - so perhaps this patch
> isn't quite perfect (and my C skills are very limited, so the
> whole thing could use a review).

No worries.  I think the following should work:

-----8<------
Subject: [PATCH] SQUASH/WIP - use rb_funcallv to handle second respond_to arg

While we're at it, avoid mixing declarations and code
in case there's still compiler compatibility problems.

(We will add a check for -Wdeclaration-after-statement support
 in a separate commit)
---
	Also pushed to the respond_to-priv branch at
	git://bogomips.org/clogger
	commit 7b3ed7c0bed876efe5298232a49f8542b8b340a0

 Do you think you can write a test?  No obligation, I can take
 care of it, too.  Thanks again!

 ext/clogger_ext/clogger.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c
index 622c98c..daed91a 100644
--- a/ext/clogger_ext/clogger.c
+++ b/ext/clogger_ext/clogger.c
@@ -965,16 +965,19 @@ static VALUE clogger_init_copy(VALUE clone, VALUE orig)
  */
 static VALUE respond_to(int argc, VALUE *argv, VALUE self)
 {
-	VALUE method, include_all;
-	rb_scan_args(argc, argv, "11", &method, &include_all);
-	if (NIL_P(include_all)) include_all = Qfalse;
-
 	struct clogger *c = clogger_get(self);
-	ID id = rb_to_id(method);
+	VALUE method, include_all;
+	ID id;
 
+	rb_scan_args(argc, argv, "11", &method, &include_all);
+	id = rb_to_id(method);
 	if (close_id == id)
 		return Qtrue;
-	return rb_respond_to(c->body, id);
+
+	if (argc == 1)
+		return rb_respond_to(c->body, id);
+
+	return rb_funcallv(c->body, respond_to_id, argc, argv);
 }
 
 /*
-- 
EW

Re: [PATCH] Update respond_to? calls for second argument.

[Eric Wong]

Actually, there's also a rb_obj_respond_to API in Ruby 1.9+
which could be used.  It's declared in ruby/intern.h which is a
grey area as far as continued API support goes, and it's not
documented in doc/extension.rdoc, either.

However, there is a rubyspec CAPI test for it; and I'm not sure
the two-arg form of respond_to? is actually used by real Rack
servers.


Sidenote: rb_funcall* functions are always a bit slower since
they need to go through method lookup before dispatch, and can't
benefit from inline method caching, only global method caching.

Re: [PATCH] Update respond_to? calls for second argument.

[Pat Allan]

For reference, here’s the point where Rack became explicit about using the two arguments:
https://github.com/rack/rack/commit/5f808aa2099841e5daec6cb772a304797879ce6c

I’m not quite sure where to start with a test, I’m afraid - so if you’re able to take care of that, that’d be brilliant.

As for the C internals - I’m reading what you’ve noted, and I’m understanding at a basic level, but you’ve got the deep knowledge here, so I’m very happy to go with your decisions :)

> On 21 May 2017, at 2:54 pm, Eric Wong <e@80x24.org> wrote:
> 
> Actually, there's also a rb_obj_respond_to API in Ruby 1.9+
> which could be used.  It's declared in ruby/intern.h which is a
> grey area as far as continued API support goes, and it's not
> documented in doc/extension.rdoc, either.
> 
> However, there is a rubyspec CAPI test for it; and I'm not sure
> the two-arg form of respond_to? is actually used by real Rack
> servers.
> 
> 
> Sidenote: rb_funcall* functions are always a bit slower since
> they need to go through method lookup before dispatch, and can't
> benefit from inline method caching, only global method caching.

[PATCH v2] Update respond_to? calls for second argument.

[Eric Wong]

Pat Allan <pat@freelancing-gods.com> wrote:
> For reference, here’s the point where Rack became explicit about using the two arguments:
> https://github.com/rack/rack/commit/5f808aa2099841e5daec6cb772a304797879ce6c

Thanks!  It looks like I never noticed that since I always have
Clogger at the outermost layer (to give the most accurate timings),
so BodyProxy would never see it.

> I’m not quite sure where to start with a test, I’m afraid - so
> if you’re able to take care of that, that’d be brilliant.

OK, done (see below)

> As for the C internals - I’m reading what you’ve noted, and
> I’m understanding at a basic level, but you’ve got the deep
> knowledge here, so I’m very happy to go with your decisions :)

No worries, feel free to ping me (+cc ruby-core) if you need
Ruby C API help.

Also, I guess rb_obj_respond_to should be documented as a public
API in Ruby.  Do you think you can open an issue on
https://bugs.ruby-lang.org/ about it that effect?  I admit I'm
a bit clumsy when it comes to browsers, even with w3m :x

Here's the version I'll push out and release:
------8<-------
From: Pat Allan <pat@freelancing-gods.com>
Subject: [PATCH] Update respond_to? calls for second argument.

Rack (since v2) has started explicitly listing the second
(optional) argument for respond_to?, which matches the
underlying Ruby spec. This patch fixes the calls in both C
and Ruby approaches.

[ew: add test, use rb_obj_respond_to if available]
---
 ext/clogger_ext/clogger.c    | 17 +++++++++++++----
 ext/clogger_ext/extconf.rb   |  1 +
 lib/clogger/pure.rb          |  4 ++--
 test/test_clogger_to_path.rb |  9 +++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/ext/clogger_ext/clogger.c b/ext/clogger_ext/clogger.c
index 481dd61..fdc23e3 100644
--- a/ext/clogger_ext/clogger.c
+++ b/ext/clogger_ext/clogger.c
@@ -963,14 +963,23 @@ static VALUE clogger_init_copy(VALUE clone, VALUE orig)
  * used to delegate +:to_path+ checks for Rack webservers that optimize
  * static file serving
  */
-static VALUE respond_to(VALUE self, VALUE method)
+static VALUE respond_to(int argc, VALUE *argv, VALUE self)
 {
 	struct clogger *c = clogger_get(self);
-	ID id = rb_to_id(method);
+	VALUE method, include_all;
+	ID id;
 
+	rb_scan_args(argc, argv, "11", &method, &include_all);
+	id = rb_to_id(method);
 	if (close_id == id)
 		return Qtrue;
-	return rb_respond_to(c->body, id);
+
+#ifdef HAVE_RB_OBJ_RESPOND_TO
+	return rb_obj_respond_to(c->body, id, RTEST(include_all));
+#endif
+	if (argc == 1)
+		return rb_respond_to(c->body, id);
+	return rb_funcallv(c->body, respond_to_id, argc, argv);
 }
 
 /*
@@ -1044,7 +1053,7 @@ void Init_clogger_ext(void)
 	rb_define_method(cClogger, "wrap_body?", clogger_wrap_body, 0);
 	rb_define_method(cClogger, "reentrant?", clogger_reentrant, 0);
 	rb_define_method(cClogger, "to_path", to_path, 0);
-	rb_define_method(cClogger, "respond_to?", respond_to, 1);
+	rb_define_method(cClogger, "respond_to?", respond_to, -1);
 	rb_define_method(cClogger, "body", body, 0);
 	CONST_GLOBAL_STR(REMOTE_ADDR);
 	CONST_GLOBAL_STR(HTTP_X_FORWARDED_FOR);
diff --git a/ext/clogger_ext/extconf.rb b/ext/clogger_ext/extconf.rb
index 523b314..b2c0891 100644
--- a/ext/clogger_ext/extconf.rb
+++ b/ext/clogger_ext/extconf.rb
@@ -22,6 +22,7 @@ begin
   have_func('rb_thread_call_without_gvl', 'ruby/thread.h')
   have_func('rb_thread_blocking_region', 'ruby.h')
   have_func('rb_thread_io_blocking_region', 'ruby.h')
+  have_func('rb_obj_respond_to', 'ruby/intern.h')
   create_makefile('clogger_ext')
 rescue Object => err
   warn "E: #{err.inspect}"
diff --git a/lib/clogger/pure.rb b/lib/clogger/pure.rb
index 77f81b4..fddfe79 100644
--- a/lib/clogger/pure.rb
+++ b/lib/clogger/pure.rb
@@ -77,8 +77,8 @@ class Clogger
     @logger.respond_to?(:fileno) ? @logger.fileno : nil
   end
 
-  def respond_to?(m)
-    :close == m.to_sym || @body.respond_to?(m)
+  def respond_to?(method, include_all=false)
+    :close == method.to_sym || @body.respond_to?(method, include_all)
   end
 
   def to_path
diff --git a/test/test_clogger_to_path.rb b/test/test_clogger_to_path.rb
index b437695..f74b991 100644
--- a/test/test_clogger_to_path.rb
+++ b/test/test_clogger_to_path.rb
@@ -14,6 +14,10 @@ class MyBody < Struct.new(:to_path, :closed)
   def close
     self.closed = true
   end
+
+private
+  def privtest
+  end
 end
 
 class TestCloggerToPath < Test::Unit::TestCase
@@ -59,6 +63,11 @@ class TestCloggerToPath < Test::Unit::TestCase
     status, headers, body = app.call(@req)
     assert_instance_of(Clogger, body)
     check_body(body)
+
+    assert ! body.respond_to?(:privtest)
+    assert body.respond_to?(:privtest, true)
+    assert ! body.respond_to?(:privtest, false)
+
     assert logger.string.empty?
     assert_equal tmp.path, body.to_path
     body.close
-- 
EW