clogger RubyGem user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
Search results ordered by [date|relevance]  view[summary|nested|Atom feed]
thread overview below | download mbox.gz: |
* [PATCH] Update respond_to? calls for second argument.
@ 2017-05-21  4:01  7% Pat Allan
  2017-05-21  4:38  8% ` Eric Wong
  0 siblings, 1 reply; 6+ results
From: Pat Allan @ 2017-05-21  4:01 UTC (permalink / raw)
  To: clogger-public

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



^ permalink raw reply related	[relevance 7%]

* Re: [PATCH] Update respond_to? calls for second argument.
  2017-05-21  4:01  7% [PATCH] Update respond_to? calls for second argument Pat Allan
@ 2017-05-21  4:38  8% ` Eric Wong
  2017-05-21  4:54  9%   ` Eric Wong
  0 siblings, 1 reply; 6+ results
From: Eric Wong @ 2017-05-21  4:38 UTC (permalink / raw)
  To: Pat Allan; +Cc: clogger-public

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

^ permalink raw reply related	[relevance 8%]

* Re: [PATCH] Update respond_to? calls for second argument.
  2017-05-21  4:38  8% ` Eric Wong
@ 2017-05-21  4:54  9%   ` Eric Wong
  2017-05-21  5:10  9%     ` Pat Allan
  0 siblings, 1 reply; 6+ results
From: Eric Wong @ 2017-05-21  4:54 UTC (permalink / raw)
  To: Pat Allan; +Cc: clogger-public

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.

^ permalink raw reply	[relevance 9%]

* Re: [PATCH] Update respond_to? calls for second argument.
  2017-05-21  4:54  9%   ` Eric Wong
@ 2017-05-21  5:10  9%     ` Pat Allan
  2017-05-21  5:47 14%       ` [PATCH v2] " Eric Wong
  0 siblings, 1 reply; 6+ results
From: Pat Allan @ 2017-05-21  5:10 UTC (permalink / raw)
  To: Eric Wong; +Cc: clogger-public

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.


^ permalink raw reply	[relevance 9%]

* [PATCH v2] Update respond_to? calls for second argument.
  2017-05-21  5:10  9%     ` Pat Allan
@ 2017-05-21  5:47 14%       ` Eric Wong
  0 siblings, 0 replies; 6+ results
From: Eric Wong @ 2017-05-21  5:47 UTC (permalink / raw)
  To: Pat Allan; +Cc: clogger-public

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

^ permalink raw reply related	[relevance 14%]

* [ANN] clogger 2.2.0 - configurable request logging for Rack
@ 2017-05-23  8:46  9% Eric Wong
  0 siblings, 0 replies; 6+ results
From: Eric Wong @ 2017-05-23  8:46 UTC (permalink / raw)
  To: clogger-public, ruby-talk; +Cc: Pat Allan

Clogger is Rack middleware for logging HTTP requests.  The log format
is customizable so you can specify exactly which fields to log.

* https://bogomips.org/clogger/
* mail archives: https://bogomips.org/clogger-public/
* email us: clogger-public@bogomips.org (publically archived)
* git clone https://bogomips.org/clogger.git
* git clone git://bogomips.org/clogger
* https://bogomips.org/clogger/NEWS.atom.xml

Changes:

    clogger v2.2.0 - Rack 2.x compatibility fix
    
    This release fixes a Rack compatibility problem when
    Rack::BodyProxy wraps the Clogger object and calls
    "respond_to?" with two arguments.  This affects folks
    who put Clogger at lower levels of the middleware stack
    (below middlewares which use Rack::BodyProxy)
    
    A huge thanks to Pat Allan for coming up with this fix.
    
    Note, the recommended usage of clogger middleware is to have
    it at the outermost layer of the Rack middleware stack where
    it can give the most accurate $request_time measurement.
    
    There's also a couple of tiny internal improvements
    around the build and miniscule GC overhead reduction.
    
    Pat Allan (1):
          Update respond_to? calls for second argument.
    
    Eric Wong (3):
          clogger.c: comment to explain the lack of GC guard
          ext: reduce frozen string marking overhead
          build: remove build-time olddoc dependency
-- 
clogger v2.2.0

^ permalink raw reply	[relevance 9%]

Results 1-6 of 6 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2017-05-21  4:01  7% [PATCH] Update respond_to? calls for second argument Pat Allan
2017-05-21  4:38  8% ` Eric Wong
2017-05-21  4:54  9%   ` Eric Wong
2017-05-21  5:10  9%     ` Pat Allan
2017-05-21  5:47 14%       ` [PATCH v2] " Eric Wong
2017-05-23  8:46  9% [ANN] clogger 2.2.0 - configurable request logging for Rack Eric Wong

Code repositories for project(s) associated with this public inbox

	https://yhbt.net/clogger.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).