http.rb gem specs fail on JRuby 9.0.0.0.pre2 and greater #3151
Comments
|
It’s probably worth noting that this issue can also be reproduced by cloning the |
|
@sferik I see 3 failures in http.rb: but I see no errors on HEAD: well perhaps other than the coverage number. I think your private error was fixed via #3123 (commit 8ae5cb3), but we apparently still have an error in http.rb. |
|
Doubleteam! I'm looking into http.rb issues now too. |
|
Well this is odd... I added some logging to delegate.rb where it uses |
|
Ok, the problem seems to be related to http.rb double-delegating the request. This code delegates twice, wrapping the given HTTP::Request in a Caching wrapper in both the def conditional_on_changes_to(cached_response)
self.class.new HTTP::Request.new(
verb, uri, headers.merge(conditional_headers_for(cached_response)),
proxy, body, version).caching
endWhy this is double-delegating is a separate question, but something about this setup seems to cause issues dispatching. When I remove one or the other delegate wrapper, the code works fine. @sferik Is this code supposed to double-delegate? |
|
Reduced repro at this point, fwiw: require 'http.rb'
request = HTTP::Request.new(:get, "http://example.com/")
subject = HTTP::Request::Caching.new(request)
mod_date = Time.now.httpdate
headers = {"Etag" => "foo", "Last-Modified" => mod_date}
cached_resp = HTTP::Response.new(200, "http/1.1", headers, "")
cond_req = subject.conditional_on_changes_to(cached_resp)
p cond_req.headers["If-None-Match"] |
|
Ok, there's a complicated set of issues at play here, but I have a repro and at least a basic understanding of the problem. HTTP::Request::Caching is a DelegateClass around HTTP::Request. When the DelegateClass(HTTP::Request) is created, it is before the full HTTP::Request class is loaded, meaning that most of its methods don't get direct delegation and instead go through method_missing. The double delegation falls into the m_m logic, where it tries to respond_to? on the inner delegate. This fails to see the method, even though it should be there as a delegation. I suspect this is ultimately a respond_to? + respond_to_missing? protocol problem. Here's the repro isolated from http.rb: class MyClass
end
class MyDel < DelegateClass(MyClass)
end
class MyClass
include Enumerable
end
MyDel.new(MyDel.new(MyClass.new)).each_with_index # BOOM@sferik I don't think you want to double-delegate and you probably should modify the creation of the HTTP::Request::Caching delegator so it happens after HTTP::Request is fully defined; the m_m logic will be significantly slower. These are not bugs, per se, but they will certainly impact performance. |
headius
changed the title
|
A slightly smaller repro that shows this is a respond_to? problem: ...
p MyDel.new(MyClass.new).respond_to? :each_with_index |
|
Bingo. I have it. diff --git a/core/src/main/java/org/jruby/RubyKernel.java b/core/src/main/java/org/jruby/RubyKernel.java
index 63837f1..c33b3fe 100644
--- a/core/src/main/java/org/jruby/RubyKernel.java
+++ b/core/src/main/java/org/jruby/RubyKernel.java
@@ -600,12 +600,12 @@ public class RubyKernel {
return RubyArgsFile.readlines(context, context.runtime.getArgsFile(), args);
}
- @JRubyMethod(name = "respond_to_missing?", module = true)
+ @JRubyMethod(name = "respond_to_missing?", module = true, visibility = PRIVATE)
public static IRubyObject respond_to_missing_p(ThreadContext context, IRubyObject recv, IRubyObject symbol) {
return context.runtime.getFalse();
}
- @JRubyMethod(name = "respond_to_missing?", module = true)
+ @JRubyMethod(name = "respond_to_missing?", module = true, visibility = PRIVATE)
public static IRubyObject respond_to_missing_p(ThreadContext context, IRubyObject recv, IRubyObject symbol, IRubyObject isPrivate) {
return context.runtime.getFalse();
}The problem goes like this:
This only affects cases passing through Delegate's Fix and specs coming. |
enebo
changed the title
|
@sferik I'll file a bug on http.rb and perhaps it will magically get fixed! |
2792cbd Fix Thread#priority= spec to follow MRI behavior.
348ee7e Remove trailing spaces
6173510 Add additional spec for Thread#priority=.
e43dd4e Remove extra top-level constants markers.
500347b Add two specs for error cases of define_method with a Method.
62def58 Make sure threads are actually waiting in Mutex specs
baf2858 Time#now should return Time instance when called on non-descendant class
8eaf6fa add test for backtrace presence
b4ea34c Add specs to Hash#merge with small hashes
13f2187 Make sure threads are actually waiting in other ConditionVariable specs
ae7a36c Make sure the thread is actually waiting in ConditionVariable specs
5c3d18e Add specs to Hash#merge with empty hashes
0133591 Add specs for Date#{<<,prev_year,next_year} around calendar reforms.
f56a046 Kernel#respond_to? and Kernel#respond_to_missing? should only be instance methods.
adf8655 Spec for #3151 DelegateClass + respond_to_missing?
0baee92 Deduplicate Kernel#puts and IO#puts, the former delegates to the latter
f46e649 Improve IO#puts specs to not rely on individual calls to #write
b3fc7ae Add spec for non-local return bubbling through lambda.
f74566e Added failing Range#bsearch specs
109c494 Add specs for passing strings to constant visibility methods
6f28f90 Smash another racey process spec.
c6d9f6a Merge pull request #100 from jannishuebl/master
57064f9 Merge pull request #101 from ruby/hash
2b58c26 restart travis
3833763 remove another filterable opal exeptions.
06ee233 remove filterable opal exeptions.
bc6c119 Add unsuppored for Opal
bf46caa Add more Hash inequality examples
8350bad fix Bignum tests for opal
a9e6eb7 fix Fixnum#* overflow to bignum for all kinds of fixnums
979cebe fix Bignum#div Bignum#divmod and Bignum#divide to run with opal bignum
dd95805 Mocks can mock #hash and #eql? now.
6f4f099 Do not make 2 builds on Travis for branch + PR
9365015 Merge pull request #99 from ruby/array-add-missing-examples
15077ea Use at_least(1) instead of any_number_of_times, add comment re `eql?`
11476a4 Port missing examples from Array#uniq to Array#uniq!
87af503 Add missing examples for identical item for Array#{&,|,uniq,uniq!}
11146c9 Merge pull request #95 from ruby/array
c8b9498 Use should_receive to test Array#& and Array#| instead of singleton methods
61eff71 Revert "Try the new Travis CI infrastructure"
82f22e7 Try the new Travis CI infrastructure
b7605dd Module#prepend is private before Ruby 2.1
08de395 Binding#local_variable_set is available from Ruby 2.1
git-subtree-dir: spec/ruby
git-subtree-split: 2792cbd1159705ecf68776fdac98d4e7096aec7e
I wasn’t able to create a reduced version, so here are the full reproduction steps:
git clone https://github.com/sferik/twitter.gitcd twittergem install bundlerbundle installbundle exec rake spec468 specs will fail with an error message similar to:
On all supported versions of CRuby, these specs all pass. These specs also pass on JRuby 1.7.21 and JRuby 9.0.0.0 up to the
pre2release, so this regression must have been introduced some time betweenpre1andpre2, which narrows it down to just 2,852 commits. 😉I hope helps you to identify and fix this issue before the JRuby 9.0.0.0 final release. Sorry for not testing sooner.
FWIW, most of the other gems I’ve tests pass with flying colors on JRuby 9.0.0.0.rc2.
The text was updated successfully, but these errors were encountered: