[fix] [pulsar-client] Fix pendingLookupRequestSemaphore leak when channel inactive by hezhangjian · Pull Request #17856 · apache/pulsar

hezhangjian · 2022-09-27T12:38:00Z

Motivation

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java

Lines 282 to 297 in b89c145

    
           public void channelInactive(ChannelHandlerContext ctx) throws Exception { 
        
               super.channelInactive(ctx); 
        
               log.info("{} Disconnected", ctx.channel()); 
        
               if (!connectionFuture.isDone()) { 
        
                   connectionFuture.completeExceptionally(new PulsarClientException("Connection already closed")); 
        
               } 
        
               ConnectException e = new ConnectException( 
        
                       "Disconnected from server at " + ctx.channel().remoteAddress()); 
        
               // Fail out all the pending ops 
        
               pendingRequests.forEach((key, future) -> { 
        
                   if (pendingRequests.remove(key, future) && !future.isDone()) { 
        
                       future.completeExceptionally(e); 
        
                   } 
        
               });

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

Modifications

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

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 · 2022-10-11T10:54:20Z

@codelipenghui @merlimat @eolivelli PTAL, thanks

codecov-commenter · 2022-10-12T05:46:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b89c145). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #17856   +/-   ##
=========================================
  Coverage          ?   45.77%           
  Complexity        ?    17536           
=========================================
  Files             ?     1573           
  Lines             ?   128218           
  Branches          ?    14100           
=========================================
  Hits              ?    58691           
  Misses            ?    63475           
  Partials          ?     6052

Flag	Coverage Δ
unittests	`45.77% <0.00%> (?)`

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

eolivelli

very good
I have left some minor comments about the test
good catch !

hezhangjian · 2022-10-12T07:25:16Z

@eolivelli I have done addressed your comments. PTAL again. Thanks

merlimat

👍

hezhangjian · 2022-10-14T01:26:44Z

/pulsarbot run-failure-checks

eolivelli

LGTM

hezhangjian · 2022-10-14T08:18:08Z

/pulsarbot run-failure-checks

…nnel inactive (#17856) ### Motivation https://github.com/apache/pulsar/blob/b89c1451551a6bbe681465726906a2e61c9d8a69/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L282-L297 The `pendingLookupRequestSemaphore` will leak when channel inactive. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests` ### Modifications We can't easily release the semaphore in `channelInactive`, because there are not only `LookUpRequest`. So release the semaphore when connectionException ### 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) - [x] `doc-not-needed` bug fixs, no need doc - [ ] `doc` (Your PR contains doc changes) - [ ] `doc-complete` (Docs have been already added)

#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  - [ ] `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)

…nnel inactive (#17856) ### Motivation https://github.com/apache/pulsar/blob/b89c1451551a6bbe681465726906a2e61c9d8a69/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L282-L297 The `pendingLookupRequestSemaphore` will leak when channel inactive. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests` ### Modifications We can't easily release the semaphore in `channelInactive`, because there are not only `LookUpRequest`. So release the semaphore when connectionException ### 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) - [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 b451880)

#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  - [ ] `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)

…nnel inactive (#17856) ### Motivation https://github.com/apache/pulsar/blob/b89c1451551a6bbe681465726906a2e61c9d8a69/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L282-L297 The `pendingLookupRequestSemaphore` will leak when channel inactive. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests` ### Modifications We can't easily release the semaphore in `channelInactive`, because there are not only `LookUpRequest`. So release the semaphore when connectionException ### 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) - [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 b451880)

#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  - [ ] `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)

…nnel inactive (#17856) ### Motivation https://github.com/apache/pulsar/blob/b89c1451551a6bbe681465726906a2e61c9d8a69/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L282-L297 The `pendingLookupRequestSemaphore` will leak when channel inactive. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests` ### Modifications We can't easily release the semaphore in `channelInactive`, because there are not only `LookUpRequest`. So release the semaphore when connectionException ### 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) - [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 b451880)

#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  - [ ] `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)

…nnel inactive (apache#17856) ### Motivation https://github.com/apache/pulsar/blob/b89c1451551a6bbe681465726906a2e61c9d8a69/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L282-L297 The `pendingLookupRequestSemaphore` will leak when channel inactive. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests` ### Modifications We can't easily release the semaphore in `channelInactive`, because there are not only `LookUpRequest`. So release the semaphore when connectionException ### 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) - [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 b451880) (cherry picked from commit 28e4b9c)

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  - [ ] `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)

…nnel inactive (apache#17856) ### Motivation https://github.com/apache/pulsar/blob/b89c1451551a6bbe681465726906a2e61c9d8a69/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L282-L297 The `pendingLookupRequestSemaphore` will leak when channel inactive. There are `LookUpRequestSemaphore` not released when removing it from `pendingRequests` ### Modifications We can't easily release the semaphore in `channelInactive`, because there are not only `LookUpRequest`. So release the semaphore when connectionException ### 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) - [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 b451880) (cherry picked from commit 28e4b9c)

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  - [ ] `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)

#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  - [ ] `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)

hezhangjian requested review from codelipenghui, eolivelli and merlimat September 27, 2022 12:38

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 27, 2022

codelipenghui assigned hezhangjian Oct 10, 2022

codelipenghui added this to the 2.12.0 milestone Oct 10, 2022

codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/client labels Oct 10, 2022

codelipenghui reviewed Oct 10, 2022

View reviewed changes

hezhangjian force-pushed the fix-semaphore-leak branch from 3c0c885 to 54afb2d Compare October 11, 2022 03:29

address some comments

52bb499

hezhangjian force-pushed the fix-semaphore-leak branch from 54afb2d to 52bb499 Compare October 11, 2022 06:02

codelipenghui approved these changes Oct 12, 2022

View reviewed changes

codelipenghui added the ready-to-test label Oct 12, 2022

codelipenghui closed this Oct 12, 2022

codelipenghui reopened this Oct 12, 2022

codelipenghui added release/2.9.4 release/2.11.1 release/2.10.3 labels Oct 12, 2022

hezhangjian changed the title ~~[pulsar-client] Fix pendingLookupRequestSemaphore leak when channel inactive~~ [fix] [pulsar-client] Fix pendingLookupRequestSemaphore leak when channel inactive Oct 12, 2022

eolivelli requested changes Oct 12, 2022

View reviewed changes

Comment thread pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientCnxTest.java Outdated

Comment thread pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientCnxTest.java Outdated

address comments

4c0e89b

eolivelli reviewed Oct 13, 2022

View reviewed changes

Comment thread pulsar-client/src/test/java/org/apache/pulsar/client/impl/ClientCnxTest.java Outdated

address comments

b9ebc0e

merlimat approved these changes Oct 14, 2022

View reviewed changes

eolivelli approved these changes Oct 14, 2022

View reviewed changes

Jason918 approved these changes Oct 14, 2022

View reviewed changes

hezhangjian merged commit b451880 into apache:master Oct 14, 2022

hezhangjian deleted the fix-semaphore-leak branch October 14, 2022 11:15

Technoboy- added cherry-picked/branch-2.11 and removed release/2.11.1 labels Oct 17, 2022

Technoboy- modified the milestones: 2.12.0, 2.11.0 Oct 17, 2022

hezhangjian mentioned this pull request Oct 27, 2022

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

Merged

4 tasks

congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 8, 2022

liangyepianzhou added a commit that referenced this pull request Dec 7, 2022

[cherry-pick][branch-2.10]Add fix cherry-pick #17856 imported.

7708693

liangyepianzhou added the cherry-picked/branch-2.10 label Dec 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fix] [pulsar-client] Fix pendingLookupRequestSemaphore leak when channel inactive - #17856

[fix] [pulsar-client] Fix pendingLookupRequestSemaphore leak when channel inactive#17856
hezhangjian merged 3 commits into
apache:masterfrom
hezhangjian:fix-semaphore-leak

hezhangjian commented Sep 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hezhangjian commented Oct 11, 2022

Uh oh!

codecov-commenter commented Oct 12, 2022 •

edited

Loading

Uh oh!

eolivelli left a comment

Uh oh!

Uh oh!

Uh oh!

hezhangjian commented Oct 12, 2022

Uh oh!

Uh oh!

merlimat left a comment

Uh oh!

hezhangjian commented Oct 14, 2022

Uh oh!

eolivelli left a comment

Uh oh!

hezhangjian commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

	public void channelInactive(ChannelHandlerContext ctx) throws Exception {
	super.channelInactive(ctx);
	log.info("{} Disconnected", ctx.channel());
	if (!connectionFuture.isDone()) {
	connectionFuture.completeExceptionally(new PulsarClientException("Connection already closed"));
	}

	ConnectException e = new ConnectException(
	"Disconnected from server at " + ctx.channel().remoteAddress());

	// Fail out all the pending ops
	pendingRequests.forEach((key, future) -> {
	if (pendingRequests.remove(key, future) && !future.isDone()) {
	future.completeExceptionally(e);
	}
	});

Uh oh!

Conversation

hezhangjian commented Sep 27, 2022

Motivation

Modifications

Verifying this change

Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hezhangjian commented Oct 11, 2022

Uh oh!

codecov-commenter commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hezhangjian commented Oct 12, 2022

Uh oh!

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

hezhangjian commented Oct 14, 2022

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

hezhangjian commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov-commenter commented Oct 12, 2022 •

edited

Loading