[fix][client] Memory leak during GET_LAST_MESSAGE_ID command processing. #301
Conversation
## Motivation `ClientConnection::newGetLastMessageId` method uses `ClientConnection::sendRequestWithId` one to send request. It adds a new node into `ClientConnection::pendingRequests_` map. However `ClientConnection::handleGetLastMessageIdResponse` method does not affect this map. As a result the size of mentioned map is increased every time when `Reader::getLastMessageIdAsync` method is called. ## Modification - `ClientConnection::newGetLastMessageId` method use now `ClientConnection::sendCommand` one to send request.
|
This change partially reverts commit 8735103 probably re-injecting timeout issue it fixed. |
There was a problem hiding this comment.
Just as @Radrik5 mentioned, it reverts the fix of apache/pulsar#12586.
I think a better solution is to remove the pendingGetLastMessageIdRequests_ map and remove it from pendingRequests_ in handleGetLastMessageIdResponse.
|
Could you address my comment? I will wait for some time. If not, I might open another PR to fix it since I'm going to cut the 3.3.0 release soon (maybe the next week) |
|
Yes, I'll correct the solution soon. Sorry for the delay. |
## Motivation `ClientConnection::newGetLastMessageId` method uses `ClientConnection::sendRequestWithId` one to send request. It adds a new node into `ClientConnection::pendingRequests_` map. However `ClientConnection::handleGetLastMessageIdResponse` method does not affect this map. As a result the size of mentioned map is increased every time when `Reader::getLastMessageIdAsync` method is called. ## Modification - `ClientConnection::newGetLastMessageId` method use now `ClientConnection::sendCommand` one to send request.
|
Sorry for late response. |
BewareMyPower
left a comment
BewareMyPower
left a comment
There was a problem hiding this comment.
Please fix the code format
|
Should I do anything else after your commit? |
No |
3 participants
Motivation
ClientConnection::newGetLastMessageIdmethod usesClientConnection::sendRequestWithIdone to send request. It adds a new node intoClientConnection::pendingRequests_map. HoweverClientConnection::handleGetLastMessageIdResponsemethod does not affect this map. As a result the size of mentioned map is increased every time whenReader::getLastMessageIdAsyncmethod is called.Modifications
ClientConnection::newGetLastMessageIdmethod usesClientConnection::sendCommandone now to send request.Verifying this change
This change is already covered by existing tests, such as ConsumerTest.testGetLastMessageId and ConsumerTest.testGetLastMessageIdBlockWhenConnectionDisconnected.
Documentation
doc-required(Your PR needs to update docs and you will update later)
[]
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)