[fix] Remove closed producers and consumers from client #2
Conversation
This file contains 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
BewareMyPower
force-pushed
the
bewaremypower/fix-close-handle
branch
from
e22d206 to
cc83de2
Compare
Fixes apache#55 ### Motivation 1. When a producer or consumer is closed, the reference is still stored in `ClientImpl`. If a client kept creating producers or consumers, the memory usage would not reduce. 2. When the `HandlerBase::connection_` field is modified, the `removeProducer` or `removeConsumer` method is not called. Then these producers and consumers will be cached in the connection until the connection is closed. 3. The `PartitionedProducerImpl` and `MultiTopicsConsumerImpl` have cyclic references, when a `Producer` or `Consumer` instance goes out of the scope, the destructors are not called. When I used GDB to debug them, I found the reference counts were both greater than 1. ### Modifications Let's use "handlers" to represent "producers and consumers". 1. In `ClientImpl`, use `SynchronizedHashMap` to store references of handlers, as well as the `cleanupXXX` methods to remove a handler. 2. Add `HandlerBase::beforeConnectionChange` method, which is called before `connection_` is modified. Disallow the access to `connection_` from derived classes. 3. Avoid `shared_from_this()` being passed into callbacks in ASIO executors for `PartitionedProducerImpl` and `MultiTopicsConsumerImpl`. This PR also unifies the `shutdown` implementations for handlers and call `shutdown` in the destructors. 1. Cancel the timers 2. Unregister itself from `ClientImpl` and `ClientConnection` 3. Set the create future with `ResultAlreadyClosed` 4. Set the state to `Closed` It's called when: - the destructor is called - `closeAsync` is completed - `unsubscribeAsync` is completed with ResultOk ### Verifications `ShutdownTest` is added to verify the following cases: - a single topic - a partitioned topic (multiple topics) - a partitioned topic with regex subscription `testClose` verifies `shutdown` when `closeAsync` and `unsubscribeAsync` are called. `testDestructor` verifies `shutdown` when handlers go out of the scope and the destructors are called.
BewareMyPower
force-pushed
the
bewaremypower/fix-close-handle
branch
from
cc83de2 to
45fe40f
Compare
BewareMyPower
added a commit
that referenced
this pull request
Sep 11, 2023
### Motivation When libcurl is used in `AuthOauth2`, the `CURLOPT_NOSIGNAL` option is not set, i.e. it will be the default value so that the `Curl_resolv_timeout` function might crash in multi-threading environment. ``` #2 0xf630 in _L_unlock_13 from /lib64/libpthread.so.0 (0x34) #3 0x2e6c7f in Curl_failf from /usr/local/bin/***/libpulsar.so (0x6f) #4 0x30a285 in Curl_resolv_timeout from /usr/local/bin/***/libpulsar.so (0x95) ``` Since there are many duplicated code when calling curl C APIs, it's hard to notice that `CURLOPT_NOSIGNAL` is not configured in `AuthOauth2`. ### Modifications Introduce a `CurlWrapper` class that sets the same options to reduce the duplicated code and adapting consistent behaviors unless a few options.
BewareMyPower
added a commit
that referenced
this pull request
Sep 13, 2023
…#313) ### Motivation When libcurl is used in `AuthOauth2`, the `CURLOPT_NOSIGNAL` option is not set, i.e. it will be the default value so that the `Curl_resolv_timeout` function might crash in multi-threading environment. ``` #2 0xf630 in _L_unlock_13 from /lib64/libpthread.so.0 (0x34) #3 0x2e6c7f in Curl_failf from /usr/local/bin/***/libpulsar.so (0x6f) #4 0x30a285 in Curl_resolv_timeout from /usr/local/bin/***/libpulsar.so (0x95) ``` Since there are many duplicated code when calling curl C APIs, it's hard to notice that `CURLOPT_NOSIGNAL` is not configured in `AuthOauth2`. ### Modifications Introduce a `CurlWrapper` class that sets the same options to reduce the duplicated code and adapting consistent behaviors unless a few options.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.