Fix for SimplePool.setPoolSize() #3143 #3144
Conversation
|
@sergebg Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@sergebg Thank you for signing the Contributor License Agreement! |
| @@ -102,11 +102,9 @@ public synchronized void setPoolSize(int poolSize) { | |||
| break; | |||
| } | |||
| T item = this.available.poll(); | |||
| if (item == null) { | |||
| this.permits.release(); | |||
There was a problem hiding this comment.
You have stopped to release permits.
Why is that, please?
There was a problem hiding this comment.
In this branch we need to call only acquire(). Original intent was to revert acquire if item==null. But this is absolutely incorrect. For example, if we have a new instance of pool, then "available" collection is empty. But setPoolSize() method should correctly update all structures. After calling setPoolSize(5) everything should contain 5. (permits==5, poolSize==5, and targetPoolSize==5). Of course, I am describing the situation when no objects were allocated. Please, check added test cases.
There was a problem hiding this comment.
I have added additional checks to test that "permits" field is correctly updated
| import org.springframework.integration.test.util.TestUtils; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.fail; |
There was a problem hiding this comment.
Please, do not rearrange imports order.
Consider to run gradlew check --parallel task to be sure that your changes follows our Checkstyle rules.
Also that will say you if your fix is valid against all the existing tests in the project.
There was a problem hiding this comment.
sorry, I tried to keep imports untouched. I will revert this
|
@garyrussell , would you mind to take a look here? Thank you! |
|
@sergebg Good catch, thanks. Some suggested test polishing here: garyrussell@3521f2a @artembilan I can squash/merge after review of that commit. |
|
Doing a local full build now; will merge after success. |
|
Cherry-picked to 5.2.x as 3850c7e |
Original implementation exits from the loop if "available" collection is empty. That is not correct, because the number of allocated items can be less then pool size. And so it's OK if "item==null". It's just required to perform cleanup if "item!=null". Otherwise it's required to complete the loop until "delta!=0".
Resolves #3143