You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by paulk-asert <gi...@git.apache.org> on 2018/03/30 23:41:03 UTC
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
GitHub user paulk-asert opened a pull request:
https://github.com/apache/groovy/pull/679
GROOVY-8525: Binary compatibility issue for GroovyClassLoader between…
… 2.4 vs later branches (alternative to PR#678)
What I haven't yet checked is whether we need to make some of the delegated method calls just throw UnsupportedOperationException in order to keep the claims like @Threadsafe.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paulk-asert/groovy groovy8525b
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/679.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #679
----
commit 78dc3c8d6111ab6365f1da519bf027d634803cce
Author: Paul King <pa...@...>
Date: 2018-03-30T23:36:57Z
GROOVY-8525: Binary compatibility issue for GroovyClassLoader between 2.4 vs later branches
----
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/679#discussion_r178419989
--- Diff: src/main/java/org/codehaus/groovy/runtime/memoize/StampedCommonCache.java ---
@@ -199,20 +208,35 @@ public int size() {
return doWithReadLock(c -> c.size());
}
+ @Override
+ public boolean isEmpty() {
+ return commonCache.isEmpty();
+ }
+
/**
* {@inheritDoc}
*/
@Override
- public V remove(final K key) {
+ public V remove(final Object key) {
return doWithWriteLock(c -> c.remove(key));
}
+ @Override
+ public void putAll(Map<? extends K, ? extends V> m) {
+
--- End diff --
The implementation is missing and should be protected by `doWithWriteLock`
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/679#discussion_r178421232
--- Diff: src/main/java/org/codehaus/groovy/runtime/memoize/StampedCommonCache.java ---
@@ -175,6 +174,11 @@ private V compute(K key, ValueProvider<? super K, ? extends V> valueProvider, bo
return doWithReadLock(c -> c.values());
}
+ @Override
+ public Set<Entry<K, V>> entrySet() {
+ return commonCache.entrySet();
--- End diff --
Implementing an interface and throwing `UnsupportedOperationException` does not look elegant...
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/679#discussion_r178421035
--- Diff: src/main/java/org/codehaus/groovy/runtime/memoize/StampedCommonCache.java ---
@@ -199,20 +208,35 @@ public int size() {
return doWithReadLock(c -> c.size());
}
+ @Override
+ public boolean isEmpty() {
+ return commonCache.isEmpty();
+ }
+
/**
* {@inheritDoc}
*/
@Override
- public V remove(final K key) {
+ public V remove(final Object key) {
return doWithWriteLock(c -> c.remove(key));
}
+ @Override
+ public void putAll(Map<? extends K, ? extends V> m) {
+
--- End diff --
Oops, I was wondering whether to lock for the whole putAll or loop and lock per entry and then forgot to supply either! I'll do the former for now.
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/679#discussion_r178422703
--- Diff: src/main/java/org/codehaus/groovy/runtime/memoize/StampedCommonCache.java ---
@@ -175,6 +174,11 @@ private V compute(K key, ValueProvider<? super K, ? extends V> valueProvider, bo
return doWithReadLock(c -> c.values());
}
+ @Override
+ public Set<Entry<K, V>> entrySet() {
+ return commonCache.entrySet();
--- End diff --
I will set aside some time to complete the exercise 😉
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/679#discussion_r178419950
--- Diff: src/main/java/org/codehaus/groovy/runtime/memoize/StampedCommonCache.java ---
@@ -175,6 +174,11 @@ private V compute(K key, ValueProvider<? super K, ? extends V> valueProvider, bo
return doWithReadLock(c -> c.values());
}
+ @Override
+ public Set<Entry<K, V>> entrySet() {
+ return commonCache.entrySet();
--- End diff --
The code should be protected by `doWithReadLock`, we should do same thing for `isEmpty` `containsValue`, etc.
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/679#discussion_r178422137
--- Diff: src/main/java/org/codehaus/groovy/runtime/memoize/StampedCommonCache.java ---
@@ -175,6 +174,11 @@ private V compute(K key, ValueProvider<? super K, ? extends V> valueProvider, bo
return doWithReadLock(c -> c.values());
}
+ @Override
+ public Set<Entry<K, V>> entrySet() {
+ return commonCache.entrySet();
--- End diff --
Inelegant but the Java collections way! ;-) Better to look inelegant rather than provide an apparently good looking but buggy implementation! In any case, I have implemented entrySet() in the same way as keys() and values() are implemented. The concern though is that if you look at ConcurrentHashMap, keySet(), entrySet() and values() are backed by views using iterators and spliterators that guarantee "weak consistency". It doesn't seem like we are making any such considerations for StampedCommonCache? I will leave that as a future exercise - we can always create such views or throw UnsupportedOperationException if we deem there is no other way to be safe.
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/679
---
[GitHub] groovy pull request #679: GROOVY-8525: Binary compatibility issue for Groovy...
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/679#discussion_r178421016
--- Diff: src/main/java/org/codehaus/groovy/runtime/memoize/StampedCommonCache.java ---
@@ -175,6 +174,11 @@ private V compute(K key, ValueProvider<? super K, ? extends V> valueProvider, bo
return doWithReadLock(c -> c.values());
}
+ @Override
+ public Set<Entry<K, V>> entrySet() {
+ return commonCache.entrySet();
--- End diff --
true, but I think we have to throw UnsupportedOperationException for entrySet and keySet because they return a view which might then be modified under the covers without locks?
---