Skip to content
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

Closed
olleolleolle opened this Issue Jul 20, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@olleolleolle
Copy link
Contributor

commented Jul 20, 2015

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: putsing 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.

@kares

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

you guys do seem to loop (map) over an array while removing elements from it at the same time, try:

stopped = @workers.dup.map { |worker| stop_worker(worker) }

still it should not NPE - so the bug is probably valid.

@olleolleolle

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

@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.

@kares

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

@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
RubyObject.java:533:in `inspect': java.lang.NullPointerException
    from RubyArray.java:1480:in `inspectAry'
    from RubyArray.java:1510:in `inspect'
    from RubyArray$INVOKER$i$0$0$inspect.gen:-1:in `call'
    from CachingCallSite.java:306:in `cacheAndCall'
    from CachingCallSite.java:136:in `call'
    from npe_array.rb:7:in `__file__'
    from npe_array.rb:-1:in `load'
    from Ruby.java:867:in `runScript'
    from Ruby.java:860:in `runScript'
    from Ruby.java:729:in `runNormally'
    from Ruby.java:578:in `runFromMain'
    from Main.java:395:in `doRunFromMain'
    from Main.java:290:in `internalRun'
    from Main.java:217:in `run'
    from Main.java:197:in `main'

... I'm not sure whether the MRI outcome (prints [2, 4]) is worth chasing down (due its "correctness")

@olleolleolle olleolleolle changed the title NullPointerException on compacting an Array NullPointerException on deleting items from an Array while mapping over it Jul 21, 2015

@headius

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

@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.

@headius

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

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"})

@headius headius closed this in 962fddd Jul 21, 2015

headius added a commit that referenced this issue Jul 21, 2015

Use iteration count for final size in map. Fixes #3155.
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.

@enebo enebo added this to the JRuby 1.7.22 milestone Jul 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.