* [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).