Fix segmentation fault when sending messages after receiving an error#326
Merged
BewareMyPower merged 3 commits intoapache:mainfrom Oct 9, 2023
Merged
Conversation
Fixes apache#325 ### Motivation apache#317 introduces a bug that might cause segmentation fault when sending messages after receiving an error, see apache#325 (comment) for the detailed explanation. ### Modifications When calling `asyncWrite`, capture the `shared_ptr` instead of the `weak_ptr` to extend the lifetime of the `socket_` or `tlsSocket_` field in `ClientConnection`. Since the lifetime is extended, in some callbacks, check `isClosed()` before other logic. Add a `ChunkDedupTest` to reproduce this issue based on Pulsar 3.1.0. Run the test for 10 times to ensure it won't crash after this patch.
2 tasks
BewareMyPower
added a commit
to streamnative/pulsar-client-cpp
that referenced
this pull request
Oct 8, 2023
…apache#326) Fixes apache#325 ### Motivation apache#317 introduces a bug that might cause segmentation fault when sending messages after receiving an error, see apache#325 (comment) for the detailed explanation. ### Modifications When calling `asyncWrite`, capture the `shared_ptr` instead of the `weak_ptr` to extend the lifetime of the `socket_` or `tlsSocket_` field in `ClientConnection`. Since the lifetime is extended, in some callbacks, check `isClosed()` before other logic. Add a `ChunkDedupTest` to reproduce this issue based on Pulsar 3.1.0. Run the test for 10 times to ensure it won't crash after this patch.
RobertIndie
approved these changes
Oct 8, 2023
tests/chunkdedup/docker-compose.yml
Outdated
| driver: bridge | ||
| services: | ||
| standalone: | ||
| image: apachepulsar/pulsar:3.1.0 # Ensure https://github.com/apache/pulsar/pull/20948 is not included |
Member
There was a problem hiding this comment.
It's better to add a comment like "Don't change the pulsar version here.". This test could only work before 3.1.0, right?
shibd
approved these changes
Oct 8, 2023
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-cpp
that referenced
this pull request
Oct 19, 2023
### Motivation apache#326 fixes the possible segmentation fault caused by async_write, but it could still crash when triggering the callback of async_receive while the socket is destroyed. See apache#324 (comment) ### Modifications Capture the `shared_ptr` in `asyncReceive`.
BewareMyPower
added a commit
that referenced
this pull request
Oct 20, 2023
### Motivation #326 fixes the possible segmentation fault caused by async_write, but it could still crash when triggering the callback of async_receive while the socket is destroyed. See #324 (comment) ### Modifications Capture the `shared_ptr` in `asyncReceive`.
BewareMyPower
added a commit
to streamnative/pulsar-client-cpp
that referenced
this pull request
Oct 20, 2023
### Motivation apache#326 fixes the possible segmentation fault caused by async_write, but it could still crash when triggering the callback of async_receive while the socket is destroyed. See apache#324 (comment) ### Modifications Capture the `shared_ptr` in `asyncReceive`. (cherry picked from commit a0f2d32)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #325
Motivation
#317 introduces a bug that might cause segmentation fault when sending messages after receiving an error, see
#325 (comment) for the detailed explanation.
Modifications
When calling
asyncWrite, capture theshared_ptrinstead of theweak_ptrto extend the lifetime of thesocket_ortlsSocket_field inClientConnection. Since the lifetime is extended, in some callbacks, checkisClosed()before other logic.Add a
ChunkDedupTestto reproduce this issue based on Pulsar 3.1.0. Run the test for 10 times to ensure it won't crash after this patch.