About NullPointerException in Recycles #355

luozhiyun993 · 2019-11-25T01:19:07Z

Motivation:

issue : #353

Modification:

Describe the idea and modifications you've done.

Result:

Fixes #.

If there is no issue then describe the changes introduced by this PR.

fengjiachun · 2019-11-25T02:35:12Z

CLA is required @Devyun

fengjiachun · 2019-11-25T02:35:34Z

And can you add some unit test?

luozhiyun993 · 2019-11-25T02:49:53Z

And can you add some unit test?

ok

fengjiachun · 2019-11-25T04:13:42Z

CLA is required @Devyun

luozhiyun993 · 2019-11-25T04:42:06Z

CLA is required @Devyun

I have signed this CLA. but clahub is still show X

fengjiachun · 2019-11-25T05:24:42Z

Running com.alipay.sofa.jraft.util.RecyclersTest
Exception in thread "Thread-6" Exception in thread "Thread-5" Exception in thread "Thread-11" java.lang.IllegalStateException: recycled already
	at com.alipay.sofa.jraft.util.Recyclers.recycle(Recyclers.java:104)
	at com.alipay.sofa.jraft.util.RecyclersTest.lambda$testRecycleMultipleAtDifferentThread$1(RecyclersTest.java:77)
	at java.lang.Thread.run(Thread.java:748)
java.lang.IllegalStateException: recycled already
	at com.alipay.sofa.jraft.util.Recyclers.recycle(Recyclers.java:104)
	at com.alipay.sofa.jraft.util.RecyclersTest.lambda$testRecycleMultipleAtDifferentThread$1(RecyclersTest.java:77)
	at java.lang.Thread.run(Thread.java:748)

ut is failed @Devyun

fengjiachun · 2019-11-25T05:26:29Z

May be we can refer netty/netty@338ef96#diff-b7c5624e42938616f2c51f7115340ae0R84

jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java

killme2008 · 2019-11-25T05:38:37Z

jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java

+
+            Stack<?> stack = this.stack;
+            if ( lastRecycledId !=  recycleId || stack == null) {
+                throw new IllegalStateException("recycled already");


jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java

fengjiachun · 2019-11-26T02:56:55Z

jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java

@@ -326,7 +329,8 @@ DefaultHandle pop() {
            size--;
            DefaultHandle ret = elements[size];
            if (ret.lastRecycledId != ret.recycleId) {
-                throw new IllegalStateException("recycled multiple times");
+                LOG.warn("recycled already");
+                return null;


pop 出现 (ret.lastRecycledId != ret.recycleId) 的情况应该是 recycle 逻辑有问题，这里并不是事故第一现场，并且这里 DefaultHandle 也没有做一些释放处理，所以我建议这里就维持刨出异常

fengjiachun · 2019-11-26T13:49:21Z

需要改一下原有的单侧： testMultipleRecycle
取消了抛出异常后这个单测就不对了

fengjiachun · 2019-11-26T13:49:38Z

@Devyun

luozhiyun993 · 2019-11-26T14:48:19Z

@Devyun

好的

killme2008 · 2019-11-27T14:10:51Z

@Devyun Please rebase this branch with master.

luozhiyun993 · 2019-11-27T15:26:40Z

@Devyun Please rebase this branch with master.

I'm done

fengjiachun · 2019-11-28T03:47:00Z

@Devyun
还有两个 test case 要麻烦你继续改下
ByteBufferCollectorTest.testMultipleRecycle
RecyclableByteBufferListTest.testMultipleRecycle

改为如下逻辑：

final RecyclableByteBufferList object = RecyclableByteBufferList.newInstance();
assertTrue(object.recycle());
assertFalse(object.recycle());

luozhiyun993 · 2019-11-28T07:52:46Z

@Devyun
还有两个 test case 要麻烦你继续改下
ByteBufferCollectorTest.testMultipleRecycle
RecyclableByteBufferListTest.testMultipleRecycle

改为如下逻辑：
final RecyclableByteBufferList object = RecyclableByteBufferList.newInstance();
assertTrue(object.recycle());
assertFalse(object.recycle());

如果要assertFalse(object.recycle());成立的话，需要改push和DefaultHandle#recycle的返回值，要改吗？

fengjiachun · 2019-11-28T08:15:12Z

@Devyun
还有两个 test case 要麻烦你继续改下
ByteBufferCollectorTest.testMultipleRecycle
RecyclableByteBufferListTest.testMultipleRecycle
改为如下逻辑：
final RecyclableByteBufferList object = RecyclableByteBufferList.newInstance();
assertTrue(object.recycle());
assertFalse(object.recycle());
如果要assertFalse(object.recycle());成立的话，需要改push和DefaultHandle#recycle的返回值，要改吗？

牵一发动全身，我觉得我们可能错了 @killme2008 ，现在我更倾向于维持原有的抛出异常的策略，本身重复 recycle() 就是个严重的错误行为，应该禁止，没必要在这里为用户的错误行为容错

fengjiachun · 2019-11-28T08:16:59Z

jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java

@@ -133,6 +140,13 @@ public final int threadLocalSize() {

        public void recycle() {
            Thread thread = Thread.currentThread();
+
+            final Stack<?> stack = this.stack;
+            if ( lastRecycledId !=  recycleId || stack == null) {


这一行代码格式要调整下， lastRecycledId 前边不应该有空格，recycleId 前边有两个空格，应该改为一个

喜欢有代码洁癖的人，哈哈哈

fengjiachun · 2019-11-28T10:16:06Z

@Devyun 我觉得要么我们还是恢复抛出异常？也方便跟进 netty 原版 code 持续更新

luozhiyun993 · 2019-11-28T10:42:36Z

@Devyun 我觉得要么我们还是恢复抛出异常？也方便跟进 netty 原版 code 持续更新

好滴~

* 'master' of https://github.com/sofastack/sofa-jraft: Feature/some changes (sofastack#358) fix elect priority (sofastack#357) Remove unused imports (sofastack#356) feat/priority_by_zongtanghu (sofastack#345)

fengjiachun · 2019-11-28T12:28:29Z

@Devyun tks

sofastack-bot bot added cla:yes First-time contributor size/XS labels Nov 25, 2019

sofastack-bot bot added size/M and removed size/XS labels Nov 25, 2019

killme2008 reviewed Nov 25, 2019

View reviewed changes

jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java Show resolved Hide resolved

killme2008 reviewed Nov 25, 2019

View reviewed changes

SteNicholas reviewed Nov 25, 2019

View reviewed changes

jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java Outdated Show resolved Hide resolved

SteNicholas reviewed Nov 25, 2019

View reviewed changes

jraft-core/src/main/java/com/alipay/sofa/jraft/util/Recyclers.java Outdated Show resolved Hide resolved

fengjiachun reviewed Nov 26, 2019

View reviewed changes

fengjiachun approved these changes Nov 26, 2019

View reviewed changes

fix bugs , issue: sofastack#353

a1db6d1

luozhiyun993 force-pushed the master branch from 4827966 to a1db6d1 Compare November 27, 2019 15:23

fengjiachun reviewed Nov 28, 2019

View reviewed changes

change reduce Space

2b056fd

luozhiyun added 2 commits November 28, 2019 18:49

change warn to exception

cb10dd7

Merge branch 'master' of https://github.com/sofastack/sofa-jraft

5f1adec

* 'master' of https://github.com/sofastack/sofa-jraft: Feature/some changes (sofastack#358) fix elect priority (sofastack#357) Remove unused imports (sofastack#356) feat/priority_by_zongtanghu (sofastack#345)

fengjiachun approved these changes Nov 28, 2019

View reviewed changes

fengjiachun merged commit 0cf0c8e into sofastack:master Nov 28, 2019

fengjiachun mentioned this pull request Nov 29, 2019

1.3.0 release #362

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

About NullPointerException in Recycles #355

About NullPointerException in Recycles #355

luozhiyun993 commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

luozhiyun993 commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

luozhiyun993 commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

killme2008 Nov 25, 2019

fengjiachun Nov 26, 2019

luozhiyun993 Nov 26, 2019

fengjiachun commented Nov 26, 2019

fengjiachun commented Nov 26, 2019

luozhiyun993 commented Nov 26, 2019

killme2008 commented Nov 27, 2019

luozhiyun993 commented Nov 27, 2019

fengjiachun commented Nov 28, 2019

luozhiyun993 commented Nov 28, 2019

fengjiachun commented Nov 28, 2019

fengjiachun Nov 28, 2019

luozhiyun993 Nov 28, 2019

fengjiachun commented Nov 28, 2019

luozhiyun993 commented Nov 28, 2019

fengjiachun commented Nov 28, 2019

About NullPointerException in Recycles #355

About NullPointerException in Recycles #355

Conversation

luozhiyun993 commented Nov 25, 2019

Motivation:

Modification:

Result:

fengjiachun commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

luozhiyun993 commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

luozhiyun993 commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

fengjiachun commented Nov 25, 2019

killme2008 Nov 25, 2019

Choose a reason for hiding this comment

fengjiachun Nov 26, 2019

Choose a reason for hiding this comment

luozhiyun993 Nov 26, 2019

Choose a reason for hiding this comment

fengjiachun commented Nov 26, 2019

fengjiachun commented Nov 26, 2019

luozhiyun993 commented Nov 26, 2019

killme2008 commented Nov 27, 2019

luozhiyun993 commented Nov 27, 2019

fengjiachun commented Nov 28, 2019

luozhiyun993 commented Nov 28, 2019

fengjiachun commented Nov 28, 2019

fengjiachun Nov 28, 2019

Choose a reason for hiding this comment

luozhiyun993 Nov 28, 2019

Choose a reason for hiding this comment

fengjiachun commented Nov 28, 2019

luozhiyun993 commented Nov 28, 2019

fengjiachun commented Nov 28, 2019