-
Notifications
You must be signed in to change notification settings - Fork 63
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][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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the code format
Should I do anything else after your commit? |
No |
Motivation
ClientConnection::newGetLastMessageId
method usesClientConnection::sendRequestWithId
one to send request. It adds a new node intoClientConnection::pendingRequests_
map. HoweverClientConnection::handleGetLastMessageIdResponse
method does not affect this map. As a result the size of mentioned map is increased every time whenReader::getLastMessageIdAsync
method is called.Modifications
ClientConnection::newGetLastMessageId
method usesClientConnection::sendCommand
one 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)