-
-
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
Map proxy modification while iterating entries #3770
Comments
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. |
This is still an issue and I don't have an answer. I lean towards making this use |
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
|
@headius 👍 this introduced changes - all for adjusting that spec (just exercises hash-like behavior too much). |
@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. |
while cleaning up Map proxies to handle all of the (old and) new
Hash
methods I stumbled across thatvisitAll
never does walk over the map's entries directly (doesentries = map.entrySet().toArray( new Map.Entry[map.size() ] );
). its likely to be closer toHash
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 :
.. 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
The text was updated successfully, but these errors were encountered: