You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/05/22 23:37:45 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request, #15707: Fix NPE when put value to `RangeCache`.
mattisonchao opened a new pull request, #15707:
URL: https://github.com/apache/pulsar/pull/15707
### Motivation
When `ReferenceCounted` object overrides the method `deallocate` to make the `getLength` value equal null will cause NPE because the `RangeCache#put` method is not thread-safe. (The process of describing this abstraction is not very clear, please refer to the code modification :)
Pulsar implementation may throw an exception to make `OpAddEntry` fail abnormal and fence the topic. relative code as below:
https://github.com/apache/pulsar/blob/defeec0e84a63ea865f3a2790bc61b66a02254c5/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java#L211-L217
**Exception screenshot:**
```java
java.lang.NullPointerException: Cannot invoke "String.length()" because "value.s" is null
at org.apache.bookkeeper.mledger.util.RangeCacheTest.lambda$testInParallel$6(RangeCacheTest.java:279)
at org.apache.bookkeeper.mledger.util.RangeCache.put(RangeCache.java:77)
at org.apache.bookkeeper.mledger.util.RangeCacheTest.testInParallel(RangeCacheTest.java:283)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:577)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599)
at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at org.testng.TestRunner.privateRun(TestRunner.java:764)
at org.testng.TestRunner.run(TestRunner.java:585)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
at org.testng.SuiteRunner.run(SuiteRunner.java:286)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
at org.testng.TestNG.runSuites(TestNG.java:1069)
at org.testng.TestNG.run(TestNG.java:1037)
at com.intellij.rt.testng.IDEARemoteTestNG.run(IDEARemoteTestNG.java:66)
at com.intellij.rt.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:109)
```
### Modifications
- Make the `RangeCache#put` method to thread-safe.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `no-need-doc`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on a diff in pull request #15707: [fix][ML]Fix NPE when put value to `RangeCache`.
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#discussion_r878992514
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -73,12 +73,11 @@ public RangeCache(Weighter<Value> weighter, TimestampExtractor<Value> timestampE
* @return whether the entry was inserted in the cache
*/
public boolean put(Key key, Value value) {
- if (entries.putIfAbsent(key, value) == null) {
+ Value v = entries.computeIfAbsent(key, (k) -> {
size.addAndGet(weighter.getSize(value));
- return true;
- } else {
- return false;
- }
+ return value;
+ });
+ return v == value;
Review Comment:
Nice catch! But look like it changed the original behavior. See the following test. Its success in the old code but failed in this change.
```java
@Test
public void test() {
RangeCache<Integer, RefString> cache = new RangeCache<>();
RefString s0 = new RefString("zero");
assertEquals(s0.refCnt(), 1);
assertTrue(cache.put(0, s0));
assertFalse(cache.put(0, s0));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ML]Fix NPE when put value to `RangeCache`. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#discussion_r1347807317
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -73,12 +74,13 @@ public RangeCache(Weighter<Value> weighter, TimestampExtractor<Value> timestampE
* @return whether the entry was inserted in the cache
*/
public boolean put(Key key, Value value) {
- if (entries.putIfAbsent(key, value) == null) {
+ MutableBoolean flag = new MutableBoolean();
+ entries.computeIfAbsent(key, (k) -> {
size.addAndGet(weighter.getSize(value));
- return true;
- } else {
- return false;
- }
+ flag.setValue(true);
+ return value;
+ });
+ return flag.booleanValue();
Review Comment:
@mattisonchao issue is #21301
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ML]Fix NPE when put value to `RangeCache`. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#discussion_r1347804842
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -73,12 +74,13 @@ public RangeCache(Weighter<Value> weighter, TimestampExtractor<Value> timestampE
* @return whether the entry was inserted in the cache
*/
public boolean put(Key key, Value value) {
- if (entries.putIfAbsent(key, value) == null) {
+ MutableBoolean flag = new MutableBoolean();
+ entries.computeIfAbsent(key, (k) -> {
size.addAndGet(weighter.getSize(value));
- return true;
- } else {
- return false;
- }
+ flag.setValue(true);
+ return value;
+ });
+ return flag.booleanValue();
Review Comment:
@mattisonchao This doesn't seem to be a thread safe change here. `computeIfAbsent` doesn't lock the key when ConcurrentSkipListMap is used.
You can see this in the source code of `ConcurrentMap`:
```
default V computeIfAbsent(K key,
Function<? super K, ? extends V> mappingFunction) {
Objects.requireNonNull(mappingFunction);
V oldValue, newValue;
return ((oldValue = get(key)) == null
&& (newValue = mappingFunction.apply(key)) != null
&& (oldValue = putIfAbsent(key, newValue)) == null)
? newValue
: oldValue;
}
```
On the other hand, ConcurrentHashMap.computeIfAbsent does lock the key and atomically call the mappingFunction. More details in a tweet where some observations by @michaeljmarshall were shared: https://twitter.com/spyced/status/1709677859764154819 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Demogorgon314 commented on a diff in pull request #15707: [fix][ML]Fix NPE when put value to `RangeCache`.
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on code in PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#discussion_r879031781
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -73,12 +74,13 @@ public RangeCache(Weighter<Value> weighter, TimestampExtractor<Value> timestampE
* @return whether the entry was inserted in the cache
*/
public boolean put(Key key, Value value) {
- if (entries.putIfAbsent(key, value) == null) {
+ AtomicBoolean flag = new AtomicBoolean(false);
Review Comment:
Better be using `MutableBoolean`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #15707: [fix][ML]Fix NPE when put value to `RangeCache`.
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #15707:
URL: https://github.com/apache/pulsar/pull/15707
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][ML]Fix NPE when put value to `RangeCache`. [pulsar]
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#discussion_r1349427552
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -73,12 +74,13 @@ public RangeCache(Weighter<Value> weighter, TimestampExtractor<Value> timestampE
* @return whether the entry was inserted in the cache
*/
public boolean put(Key key, Value value) {
- if (entries.putIfAbsent(key, value) == null) {
+ MutableBoolean flag = new MutableBoolean();
+ entries.computeIfAbsent(key, (k) -> {
size.addAndGet(weighter.getSize(value));
- return true;
- } else {
- return false;
- }
+ flag.setValue(true);
+ return value;
+ });
+ return flag.booleanValue();
Review Comment:
Yes, you are right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #15707: [fix][ML]Fix NPE when put value to `RangeCache`.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#issuecomment-1134085574
This may be a core cause of topic fenced, but we are still confirming.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #15707: [fix][ML]Fix NPE when put value to `RangeCache`.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15707:
URL: https://github.com/apache/pulsar/pull/15707#discussion_r879027523
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -73,12 +73,11 @@ public RangeCache(Weighter<Value> weighter, TimestampExtractor<Value> timestampE
* @return whether the entry was inserted in the cache
*/
public boolean put(Key key, Value value) {
- if (entries.putIfAbsent(key, value) == null) {
+ Value v = entries.computeIfAbsent(key, (k) -> {
size.addAndGet(weighter.getSize(value));
- return true;
- } else {
- return false;
- }
+ return value;
+ });
+ return v == value;
Review Comment:
Got it, very thanks to you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org