-
-
Notifications
You must be signed in to change notification settings - Fork 925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose two Enumerator-related bugs #3814
Conversation
These are definitely still true and probably related to our block arg-processing logic. We can merge this but until we fix it we'd need to tag them for JRuby. |
So, this is the commit that breaks |
Ok, so the corresponding PR for the above commit is this one and the corresponding issue
I'll add a test for this case as well and hopefully find an implementation that passes all tests. |
@@ -5,4 +5,7 @@ | |||
|
|||
describe "Enumerator::Lazy#map" do | |||
it_behaves_like :enumerator_lazy_collect, :map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an extra blank line here.
I added a spec for the bug that the commit fixes. |
@nightscape We've realized for a while we need a rework of how Enumera{ble,tor} passes values through. The current structure tends to lose the incoming argument layout and not match it properly with parameter binding. 9.1.1 might be a good time for us to look into fixing this. I'm curious: do you have an app or library failing to work because of the current bugs in Enumera{ble,tor}? |
Yes, we have a part of our code where we use Enumerators to efficiently stream data through a calculation pipeline to keep memory consumption low. We were running on MRI without issues but moved over to JRuby (we've ported parts over to Spark in order to speed up things) and suddenly things were failing in strange ways. We're not using lazy enumerators anymore now and convert the Enumerator into a real array in order to workaround this issue. |
@nightscape Were the problems you had only in lazy Enumerator or in regular Enumerator too? |
Both. We ran into the two problems that I've mentioned in the first post. They almost "cancel each other out" but then again not really ;) |
Ok, I'll try to prioritize this for the first (hopefully quick) maintenance release of 9.1. Thank you for the test cases...they will help us sort out exactly what to fix. |
Cool, thanks a lot! If I can help with anything where one doesn't have to be an expert in JRuby internals let me know! |
To chime in, I encountered this bug in a data processing script I was trying to run on JRuby. I have an understanding of this problem being caused by splats and arg destructuring in /core/src/main/ruby/jruby/kernel/enumerator.rb (and possibly RubyYielder.java?) but haven't come up with a fix. If anyone has any suggestions for an approach to fixing this, i could try to implement it and submit a PR. But as it stands, it's a bit dizzying to puzzle through. |
Sadly still broken in 9.1.3.0, even with arg fixes. We need a rework of Enumerable/Enumerator to fix this, I suspect. |
This may simply be a bit of bad behavior in the /* :nodoc: */
static VALUE
yielder_yield(VALUE obj, VALUE args)
{
struct yielder *ptr = yielder_ptr(obj);
return rb_proc_call(ptr->proc, args);
}
/* :nodoc: */
static VALUE
yielder_yield_push(VALUE obj, VALUE args)
{
yielder_yield(obj, args);
return obj;
} This patch gets your examples working. I am checking tests now. diff --git a/core/src/main/java/org/jruby/RubyYielder.java b/core/src/main/java/org/jruby/RubyYielder.java
index 6494e4d..1376db4 100644
--- a/core/src/main/java/org/jruby/RubyYielder.java
+++ b/core/src/main/java/org/jruby/RubyYielder.java
@@ -96,17 +96,13 @@ public class RubyYielder extends RubyObject {
}
@JRubyMethod(rest = true)
- public IRubyObject yield(ThreadContext context, IRubyObject[]args) {
+ public IRubyObject yield(ThreadContext context, IRubyObject[] args) {
checkInit();
return proc.call19(context, args, Block.NULL_BLOCK);
}
@JRubyMethod(name = "<<", rest = true)
- public IRubyObject op_lshift(ThreadContext context, IRubyObject[]args) {
- if (args.length == 1 &&
- args[0] instanceof RubyArray &&
- ((RubyArray) args[0]).getLength() == 1)
- args[0] = RubyArray.newArray(context.runtime, args[0]);
+ public IRubyObject op_lshift(ThreadContext context, IRubyObject[] args) {
yield(context, args);
return this;
} |
Stuffed my change into a PR at #4106. We'll see how it looks. |
Well I realized after reading @nightscape's comment above, I realized that #4106 basically would just revert #2458, which fixed #2376. It's obvious the fix isn't quite right, but we regress a different way if we remove it. |
Replacing our |
Ok, replacing both class Enumerator::Yielder
def yield(*args)
@proc.call(*args)
end
end
module Enumerable
def first
val = nil
catch(:foo) do
each do |x|
val = x
throw :foo
end
end
val
end
end
p Enumerator.new {|y| y.yield([1])}.to_a
p Enumerator.new {|y| y << [1]}.to_a
p Enumerator.new {|y| y.yield([1])}.map {|e| e}.to_a
p Enumerator.new {|y| y.yield([1])}.lazy.map {|e| e}.to_a
p Enumerator.new {|y| y.yield [1]}.first Output
This is more demonstration that our Java-based Enumerable has several hard-to-fix flaws left in it. I think we're going to need to move to a Ruby-based impl sooner rather than later. |
This PR kinda became a place to discuss the fix, but I'd like to get these specs in. I'm going to merge, tag, and open a separate bug for actually fixing the issue. |
also from comments jrubyGH-3814 which are now passing `Enumerator.new { |y| y.yield([1]) }.lazy.map { |e| e }.to_a` seems to be left not working as in MRI
JRuby seems to have two bugs related to Enumerators yielding Arrays. They can be reproduced when comparing the following code snippets which correctly return
true
for MRI, butfalse
on JRuby.<<
should be an alias for.yield
map
on lazy Enumerators should not unwrap ArraysI added corresponding specs, but I'm not sure if they're in the right place and format.