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.

Sign up for GitHub

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

Jump to bottom

Map proxy modification while iterating entries #3770

Open
kares opened this issue Mar 31, 2016 · 5 comments
Open

Map proxy modification while iterating entries #3770

kares opened this issue Mar 31, 2016 · 5 comments
Labels
internal java integration

Comments

@kares
Copy link
Member

kares commented Mar 31, 2016

while cleaning up Map proxies to handle all of the (old and) new Hash methods I stumbled across that visitAll never does walk over the map's entries directly (does entries = map.entrySet().toArray( new Map.Entry[map.size() ] );). its likely to be closer to Hash behaviour - which allows deletes while iterating (from the same thread).

have no opinion on that just wanted some discussion just in case we want to change that behavior in 9.1 :

    java.util.HashMap.new({ 1 => 1, 2 => 2 }).tap do |map|
      map.each { |key, val| map.delete(key) if val == 1 }
    end

.. currently works (like with Hash-es) - however it does not reflect how a Map behaves in Java land.

probably the only down-side of having such support baked in is harder fail-fast concurrent modification detection which Map iterators do but only other hand Map-s feel very much like a Hash 🎱

// cc @enebo @headius
@Lan5432
Copy link
Contributor

Lan5432 commented Apr 1, 2016

We have a concurrent Map like collection, right? Allowing concurrent modification on a collection that's not by definition concurrent safe may mislead some people.

Guess we'll have to see if this is actually a problem.

@kares kares changed the title Map proxies and modifications whilte iterating Map proxy modification while iterating entries Apr 6, 2016
@headius
Copy link
Member

headius commented Jul 8, 2020

This is still an issue and I don't have an answer. I lean towards making this use forEach since we now require Java 8, which would be both more efficient and more appropriate for iterating a Java map's entries. It would not support the delete behavior like a Ruby Hash, but I think that's fine since it's not a Ruby Hash (it just looks like one).

@headius
Copy link
Member

headius commented Jul 8, 2020

A patch that does what I suggest above is provided below, along with a failure it causes in JI specs.

diff --git a/core/src/main/java/org/jruby/java/proxies/MapJavaProxy.java b/core/src/main/java/org/jruby/java/proxies/MapJavaProxy.java
index f8f5cec5b2..d3a5598554 100644
--- a/core/src/main/java/org/jruby/java/proxies/MapJavaProxy.java
+++ b/core/src/main/java/org/jruby/java/proxies/MapJavaProxy.java
@@ -228,17 +228,14 @@ public final class MapJavaProxy extends ConcreteJavaProxy {
 
         @Override
         public <T> void visitAll(ThreadContext context, VisitorWithState visitor, T state) {
-            final Ruby runtime = getRuntime();
-            // NOTE: this is here to make maps act similar to Hash-es which allow modifications while
-            // iterating (meant from the same thread) ... thus we avoid iterating entrySet() directly
+            final Ruby runtime = context.runtime;
             final Map<Object, Object> map = mapDelegate();
-            final Map.Entry[] entries = map.entrySet().toArray(NULL_MAP_ENTRY);
-            int index = 0;
-            for ( Map.Entry entry : entries ) {
-                IRubyObject key = JavaUtil.convertJavaToUsableRubyObject(runtime, entry.getKey());
-                IRubyObject value = JavaUtil.convertJavaToUsableRubyObject(runtime, entry.getValue());
-                visitor.visit(context, this, key, value, index++, state);
-            }
+            int[] index = {0};
+            map.forEach((_key, _value) -> {
+                IRubyObject key = JavaUtil.convertJavaToUsableRubyObject(runtime, _key);
+                IRubyObject value = JavaUtil.convertJavaToUsableRubyObject(runtime, _value);
+                visitor.visit(context, this, key, value, index[0]++, state);
+            });
         }
 
         @Override
  1) a java.util.Map instance supports Hash-like operations
     Failure/Error: Unable to find java.util.HashMap.forEach(HashMap.java to read failed line
     Java::JavaUtil::ConcurrentModificationException:
     # java.util.HashMap.forEach(HashMap.java:1292)
     # org.jruby.java.proxies.MapJavaProxy$RubyHashMap.visitAll(MapJavaProxy.java:234)

@kares
Copy link
Member Author

kares commented Jul 9, 2020

@headius 👍 this introduced changes - all for adjusting that spec (just exercises hash-like behavior too much).
also wanted to eliminate Map proxy having to be wrapped in a RubyHash extending class, maybe some day 🎐

@headius
Copy link
Member

headius commented Jul 9, 2020

@kares Yeah I'm sure that was just a convenience, since there's a lot of logic in RubyHash. To be honest, I'd really like to see RubyHash itself work with any Map type, or perhaps we move the RubyHash logic into a extended Map implementation, and then the Ruby type just wraps that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
internal java integration
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@headius @kares @Lan5432