-
-
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
NullPointerException on deleting items from an Array while mapping over it #3155
Comments
you guys do seem to loop (
still it should not NPE - so the bug is probably valid. |
@kares Thanks for your excellent feedback. Of course it was weird to mutate-and-map an Array. Avoiding that avoided the misbehavior of JRuby. Mutate-and-map may be the key insight to build a short reproducible program for this. |
@olleolleolle thanks for confirming - yes array ops probably won't behave correctly e.g. on 1.7.21 def do_remove(v)
@array.delete(v)
end
@array = [1, 2, 3]
puts @array.map { |v| do_remove(v); v + 1 }.inspect
... I'm not sure whether the MRI outcome (prints |
@kares Good eye! I didn't notice that! I think we can fix this by reloading the RubyArrayFields each loop or something similar, and possibly warn when they change. I'll fiddle with it for a bit this morning. |
Here's a simple fix. I'm wondering if we need to audit the other Array methods to ensure they're not making assumptions about the size of the array while they iterate. diff --git a/core/src/main/java/org/jruby/RubyArray.java b/core/src/main/java/org/jruby/RubyArray.java
index f4e52c0..5b25e30 100644
--- a/core/src/main/java/org/jruby/RubyArray.java
+++ b/core/src/main/java/org/jruby/RubyArray.java
@@ -2279,13 +2279,15 @@ public class RubyArray extends RubyObject implements List, RandomAccess {
IRubyObject[] arr = new IRubyObject[realLength];
- for (int i = 0; i < realLength; i++) {
+ int i;
+ for (i = 0; i < realLength; i++) {
// Do not coarsen the "safe" check, since it will misinterpret AIOOBE from the yield
// See JRUBY-5434
arr[i] = block.yield(context, safeArrayRef(values, i + begin));
}
- return new RubyArray(runtime, arr);
+ // use iteration count as new size in case something was deleted along the way
+ return new RubyArray(runtime, arr, 0, i);
}
@JRubyMethod(name = {"collect"}) |
When the block passed to map makes modifications to the array under iteration, we may prematurely finish the map loop due to the size changing. However our logic for creating the mapped array assumed the new array's size would always be the same as the original array's size, leading to an array with null elements. This fix uses the iteration count as the final size, so we at least know how many elements in the new array were populated. Note that this behavior is officially undefined; modifying the array while performing internal iteration can cause peculiar effects across runtimes and potentially across the different versions of the same runtime. We add a regression spec here to at least make sure we don't produce an invalid array.
We are using Actors from Celluloid.
Somehow, a program of ours has a hard time compacting an Array. (I.e. using it after we'd mapped over it, while deleting items in it.)
This crash comes when we use 'finalizers' from Celluloid. Here is the relevant file (in the version we use) for that feature:
https://github.com/celluloid/celluloid/blob/0-15-stable/lib/celluloid/actor.rb
Also:
puts
ing the Array, also crashed JRuby.A backtrace of running a Celluloid program on JRuby 1.7.21 and on 9.0.0.0.rc2 and on master (0f616f984ba51ccd4d8ddafc53dd8dab93b4e5dc)
snippet.rb that tries to show what we do.
Update: See comment below for reproduce case.
The text was updated successfully, but these errors were encountered: