Skip to content
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

[fix] [pulsar-client] Fix pendingLookupRequestSemaphore leak when Ser… #18219

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

hezhangjian
Copy link
Member

Motivation

protected void handleError(CommandError error) {
checkArgument(state == State.SentConnectFrame || state == State.Ready);
log.warn("{} Received error from server: {}", ctx.channel(), error.getMessage());
long requestId = error.getRequestId();
if (error.getError() == ServerError.ProducerBlockedQuotaExceededError) {
log.warn("{} Producer creation has been blocked because backlog quota exceeded for producer topic",
ctx.channel());
}
if (error.getError() == ServerError.AuthenticationError) {
connectionFuture.completeExceptionally(
new PulsarClientException.AuthenticationException(error.getMessage()));
log.error("{} Failed to authenticate the client", ctx.channel());
}
if (error.getError() == ServerError.NotAllowedError) {
log.error("Get not allowed error, {}", error.getMessage());
connectionFuture.completeExceptionally(new PulsarClientException.NotAllowedException(error.getMessage()));
}
CompletableFuture<?> requestFuture = pendingRequests.remove(requestId);
if (requestFuture != null) {
requestFuture.completeExceptionally(
getPulsarClientException(error.getError(),
buildError(error.getRequestId(), error.getMessage())));
} else {
log.warn("{} Received unknown request id from server: {}", ctx.channel(), error.getRequestId());
}
}

The pendingLookupRequestSemaphore will leak when handleError. There are LookUpRequestSemaphore not released when removing it from pendingRequests

related PR: #17856

Modifications

We can't easily release the semaphore in handleError, because there are not only LookUpRequest. So release the semaphore when LookupException

Verifying this change

Add unit test case to cover this change

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    bug fixs, no need doc

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@hezhangjian hezhangjian added type/bug The PR fixed a bug or issue reported a bug component/client-java release/2.11.1 labels Oct 27, 2022
@hezhangjian hezhangjian self-assigned this Oct 27, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Merging #18219 (9559b78) into master (6c65ca0) will increase coverage by 14.78%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #18219       +/-   ##
=============================================
+ Coverage     34.91%   49.70%   +14.78%     
- Complexity     5707     6991     +1284     
=============================================
  Files           607      398      -209     
  Lines         53396    43548     -9848     
  Branches       5712     4474     -1238     
=============================================
+ Hits          18644    21646     +3002     
+ Misses        32119    19527    -12592     
+ Partials       2633     2375      -258     
Flag Coverage Δ
unittests 49.70% <0.00%> (+14.78%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...in/java/org/apache/pulsar/PulsarBrokerStarter.java 0.00% <ø> (ø)
.../org/apache/pulsar/PulsarClusterMetadataSetup.java 0.00% <ø> (ø)
...g/apache/pulsar/PulsarClusterMetadataTeardown.java 0.00% <ø> (ø)
...org/apache/pulsar/PulsarInitialNamespaceSetup.java 0.00% <ø> (ø)
.../main/java/org/apache/pulsar/PulsarStandalone.java 0.00% <0.00%> (ø)
...ava/org/apache/pulsar/PulsarStandaloneBuilder.java 0.00% <ø> (ø)
...apache/pulsar/broker/service/TopicListService.java 10.65% <0.00%> (-44.27%) ⬇️
...ce/schema/validator/StructSchemaDataValidator.java 52.38% <0.00%> (-9.53%) ⬇️
...g/apache/pulsar/broker/service/StreamingStats.java 75.67% <0.00%> (-8.11%) ⬇️
...tent/NonPersistentDispatcherMultipleConsumers.java 40.74% <0.00%> (-7.41%) ⬇️
... and 388 more

@hezhangjian
Copy link
Member Author

/pulsarbot run-failure-checks

@hezhangjian hezhangjian merged commit fad3ccc into apache:master Oct 27, 2022
@hezhangjian hezhangjian deleted the semephore-leak-1027 branch October 27, 2022 15:01
congbobo184 pushed a commit that referenced this pull request Nov 9, 2022
#18219)

### Motivation
https://github.com/apache/pulsar/blob/b061c6ac5833c21e483368febebd0d30679a35e1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L748-L774
The `pendingLookupRequestSemaphore` will leak when handleError. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests`

related PR: #17856

### Modifications
We can't easily release the semaphore in `handleError`, because there are not only `LookUpRequest`. So release the semaphore when LookupException

### Verifying this change
Add unit test case to cover this change

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required`
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed`
bug fixs, no need doc

- [ ] `doc`
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)

(cherry picked from commit fad3ccc)
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 9, 2022
congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
#18219)

### Motivation
https://github.com/apache/pulsar/blob/b061c6ac5833c21e483368febebd0d30679a35e1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L748-L774
The `pendingLookupRequestSemaphore` will leak when handleError. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests`

related PR: #17856

### Modifications
We can't easily release the semaphore in `handleError`, because there are not only `LookUpRequest`. So release the semaphore when LookupException

### Verifying this change
Add unit test case to cover this change

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required`
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed`
bug fixs, no need doc

- [ ] `doc`
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)

(cherry picked from commit fad3ccc)
liangyepianzhou pushed a commit that referenced this pull request Dec 14, 2022
#18219)

### Motivation
https://github.com/apache/pulsar/blob/b061c6ac5833c21e483368febebd0d30679a35e1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L748-L774
The `pendingLookupRequestSemaphore` will leak when handleError. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests`

related PR: #17856

### Modifications
We can't easily release the semaphore in `handleError`, because there are not only `LookUpRequest`. So release the semaphore when LookupException

### Verifying this change
Add unit test case to cover this change

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required`
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed`
bug fixs, no need doc

- [ ] `doc`
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)

(cherry picked from commit fad3ccc)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
apache#18219)

### Motivation
https://github.com/apache/pulsar/blob/b061c6ac5833c21e483368febebd0d30679a35e1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L748-L774
The `pendingLookupRequestSemaphore` will leak when handleError. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests`

related PR: apache#17856

### Modifications
We can't easily release the semaphore in `handleError`, because there are not only `LookUpRequest`. So release the semaphore when LookupException

### Verifying this change
Add unit test case to cover this change

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required`
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed`
bug fixs, no need doc

- [ ] `doc`
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)

(cherry picked from commit fad3ccc)
(cherry picked from commit 3996f3d)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
apache#18219)

### Motivation
https://github.com/apache/pulsar/blob/b061c6ac5833c21e483368febebd0d30679a35e1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L748-L774
The `pendingLookupRequestSemaphore` will leak when handleError. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests`

related PR: apache#17856

### Modifications
We can't easily release the semaphore in `handleError`, because there are not only `LookUpRequest`. So release the semaphore when LookupException

### Verifying this change
Add unit test case to cover this change

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required`
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed`
bug fixs, no need doc

- [ ] `doc`
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)

(cherry picked from commit fad3ccc)
(cherry picked from commit 3996f3d)
Technoboy- pushed a commit that referenced this pull request Feb 8, 2023
#18219)

### Motivation
https://github.com/apache/pulsar/blob/b061c6ac5833c21e483368febebd0d30679a35e1/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L748-L774
The `pendingLookupRequestSemaphore` will leak when handleError. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests`

related PR: #17856 

### Modifications
We can't easily release the semaphore in `handleError`, because there are not only `LookUpRequest`. So release the semaphore when LookupException

### Verifying this change
Add unit test case to cover this change

### Documentation

<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->

- [ ] `doc-required` 
(Your PR needs to update docs and you will update later)

- [x] `doc-not-needed` 
bug fixs, no need doc

- [ ] `doc` 
(Your PR contains doc changes)

- [ ] `doc-complete`
(Docs have been already added)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants