[C++] Wait until event loop terminates when closing the Client #15316
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
Fixes apache#13267 ### Motivation Unlike Java client, the `Client` of C++ client has a `shutdown` method that is responsible to execute the following steps: 1. Call `shutdown` on all internal producers and consumers 2. Close all connections in the pool 3. Close all executors of the executor providers. When an executor is closed, it call `io_service::stop()`, which makes the event loop (`io_service::run()`) in another thread return as soon as possible. However, there is no wait operation. If a client failed to create a producer or consumer, the `close` method will call `shutdown` and close all executors immediately and exits the application. In this case, the detached event loop thread might not exit ASAP, then valgrind will detect the memory leak. This memory leak can be avoided by sleeping for a while after `Client::close` returns or there are still other things to do after that. However, we should still adopt the semantics that after `Client::shutdown` returns, all event loop threads should be terminated. ### Modifications - Add a timeout parameter to the `close` method of `ExecutorService` and `ExecutorServiceProvider` as the max blocking timeout if it's non-negative. - Add a `TimeoutProcessor` helper class to update the left timeout after calling all methods that accept the timeout parameter. - Call `close` on all `ExecutorServiceProvider`s in `ClientImpl::shutdown` with 500ms timeout, which could be long enough. In addition, in `handleClose` method, call `shutdown` in another thread to avoid the deadlock. ### Verifying this change After applying this patch, the reproduce code in apache#13627 will pass the valgrind check. ``` ==3013== LEAK SUMMARY: ==3013== definitely lost: 0 bytes in 0 blocks ==3013== indirectly lost: 0 bytes in 0 blocks ==3013== possibly lost: 0 bytes in 0 blocks ```
BewareMyPower
requested review from
merlimat,
rdhabalia,
jiazhai,
aahmed-se,
k2la and
massakam
BewareMyPower
changed the title
…in different threads
lhotari
approved these changes
Apr 27, 2022
codelipenghui
pushed a commit
that referenced
this pull request
Apr 28, 2022
* [C++] Wait until event loops terminates when closing the Client Fixes #13267 ### Motivation Unlike Java client, the `Client` of C++ client has a `shutdown` method that is responsible to execute the following steps: 1. Call `shutdown` on all internal producers and consumers 2. Close all connections in the pool 3. Close all executors of the executor providers. When an executor is closed, it call `io_service::stop()`, which makes the event loop (`io_service::run()`) in another thread return as soon as possible. However, there is no wait operation. If a client failed to create a producer or consumer, the `close` method will call `shutdown` and close all executors immediately and exits the application. In this case, the detached event loop thread might not exit ASAP, then valgrind will detect the memory leak. This memory leak can be avoided by sleeping for a while after `Client::close` returns or there are still other things to do after that. However, we should still adopt the semantics that after `Client::shutdown` returns, all event loop threads should be terminated. ### Modifications - Add a timeout parameter to the `close` method of `ExecutorService` and `ExecutorServiceProvider` as the max blocking timeout if it's non-negative. - Add a `TimeoutProcessor` helper class to update the left timeout after calling all methods that accept the timeout parameter. - Call `close` on all `ExecutorServiceProvider`s in `ClientImpl::shutdown` with 500ms timeout, which could be long enough. In addition, in `handleClose` method, call `shutdown` in another thread to avoid the deadlock. ### Verifying this change After applying this patch, the reproduce code in #13627 will pass the valgrind check. ``` ==3013== LEAK SUMMARY: ==3013== definitely lost: 0 bytes in 0 blocks ==3013== indirectly lost: 0 bytes in 0 blocks ==3013== possibly lost: 0 bytes in 0 blocks ``` (cherry picked from commit cd78f39)
codelipenghui
pushed a commit
that referenced
this pull request
Apr 28, 2022
* [C++] Wait until event loops terminates when closing the Client Fixes #13267 ### Motivation Unlike Java client, the `Client` of C++ client has a `shutdown` method that is responsible to execute the following steps: 1. Call `shutdown` on all internal producers and consumers 2. Close all connections in the pool 3. Close all executors of the executor providers. When an executor is closed, it call `io_service::stop()`, which makes the event loop (`io_service::run()`) in another thread return as soon as possible. However, there is no wait operation. If a client failed to create a producer or consumer, the `close` method will call `shutdown` and close all executors immediately and exits the application. In this case, the detached event loop thread might not exit ASAP, then valgrind will detect the memory leak. This memory leak can be avoided by sleeping for a while after `Client::close` returns or there are still other things to do after that. However, we should still adopt the semantics that after `Client::shutdown` returns, all event loop threads should be terminated. ### Modifications - Add a timeout parameter to the `close` method of `ExecutorService` and `ExecutorServiceProvider` as the max blocking timeout if it's non-negative. - Add a `TimeoutProcessor` helper class to update the left timeout after calling all methods that accept the timeout parameter. - Call `close` on all `ExecutorServiceProvider`s in `ClientImpl::shutdown` with 500ms timeout, which could be long enough. In addition, in `handleClose` method, call `shutdown` in another thread to avoid the deadlock. ### Verifying this change After applying this patch, the reproduce code in #13627 will pass the valgrind check. ``` ==3013== LEAK SUMMARY: ==3013== definitely lost: 0 bytes in 0 blocks ==3013== indirectly lost: 0 bytes in 0 blocks ==3013== possibly lost: 0 bytes in 0 blocks ``` (cherry picked from commit cd78f39)
codelipenghui
pushed a commit
that referenced
this pull request
Apr 29, 2022
* [C++] Wait until event loops terminates when closing the Client Fixes #13267 ### Motivation Unlike Java client, the `Client` of C++ client has a `shutdown` method that is responsible to execute the following steps: 1. Call `shutdown` on all internal producers and consumers 2. Close all connections in the pool 3. Close all executors of the executor providers. When an executor is closed, it call `io_service::stop()`, which makes the event loop (`io_service::run()`) in another thread return as soon as possible. However, there is no wait operation. If a client failed to create a producer or consumer, the `close` method will call `shutdown` and close all executors immediately and exits the application. In this case, the detached event loop thread might not exit ASAP, then valgrind will detect the memory leak. This memory leak can be avoided by sleeping for a while after `Client::close` returns or there are still other things to do after that. However, we should still adopt the semantics that after `Client::shutdown` returns, all event loop threads should be terminated. ### Modifications - Add a timeout parameter to the `close` method of `ExecutorService` and `ExecutorServiceProvider` as the max blocking timeout if it's non-negative. - Add a `TimeoutProcessor` helper class to update the left timeout after calling all methods that accept the timeout parameter. - Call `close` on all `ExecutorServiceProvider`s in `ClientImpl::shutdown` with 500ms timeout, which could be long enough. In addition, in `handleClose` method, call `shutdown` in another thread to avoid the deadlock. ### Verifying this change After applying this patch, the reproduce code in #13627 will pass the valgrind check. ``` ==3013== LEAK SUMMARY: ==3013== definitely lost: 0 bytes in 0 blocks ==3013== indirectly lost: 0 bytes in 0 blocks ==3013== possibly lost: 0 bytes in 0 blocks ``` (cherry picked from commit cd78f39)
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
May 4, 2022
…e#15316) * [C++] Wait until event loops terminates when closing the Client Fixes apache#13267 ### Motivation Unlike Java client, the `Client` of C++ client has a `shutdown` method that is responsible to execute the following steps: 1. Call `shutdown` on all internal producers and consumers 2. Close all connections in the pool 3. Close all executors of the executor providers. When an executor is closed, it call `io_service::stop()`, which makes the event loop (`io_service::run()`) in another thread return as soon as possible. However, there is no wait operation. If a client failed to create a producer or consumer, the `close` method will call `shutdown` and close all executors immediately and exits the application. In this case, the detached event loop thread might not exit ASAP, then valgrind will detect the memory leak. This memory leak can be avoided by sleeping for a while after `Client::close` returns or there are still other things to do after that. However, we should still adopt the semantics that after `Client::shutdown` returns, all event loop threads should be terminated. ### Modifications - Add a timeout parameter to the `close` method of `ExecutorService` and `ExecutorServiceProvider` as the max blocking timeout if it's non-negative. - Add a `TimeoutProcessor` helper class to update the left timeout after calling all methods that accept the timeout parameter. - Call `close` on all `ExecutorServiceProvider`s in `ClientImpl::shutdown` with 500ms timeout, which could be long enough. In addition, in `handleClose` method, call `shutdown` in another thread to avoid the deadlock. ### Verifying this change After applying this patch, the reproduce code in apache#13627 will pass the valgrind check. ``` ==3013== LEAK SUMMARY: ==3013== definitely lost: 0 bytes in 0 blocks ==3013== indirectly lost: 0 bytes in 0 blocks ==3013== possibly lost: 0 bytes in 0 blocks ``` (cherry picked from commit cd78f39) (cherry picked from commit c0c67db)
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
May 9, 2022
…e#15316) * [C++] Wait until event loops terminates when closing the Client Fixes apache#13267 ### Motivation Unlike Java client, the `Client` of C++ client has a `shutdown` method that is responsible to execute the following steps: 1. Call `shutdown` on all internal producers and consumers 2. Close all connections in the pool 3. Close all executors of the executor providers. When an executor is closed, it call `io_service::stop()`, which makes the event loop (`io_service::run()`) in another thread return as soon as possible. However, there is no wait operation. If a client failed to create a producer or consumer, the `close` method will call `shutdown` and close all executors immediately and exits the application. In this case, the detached event loop thread might not exit ASAP, then valgrind will detect the memory leak. This memory leak can be avoided by sleeping for a while after `Client::close` returns or there are still other things to do after that. However, we should still adopt the semantics that after `Client::shutdown` returns, all event loop threads should be terminated. ### Modifications - Add a timeout parameter to the `close` method of `ExecutorService` and `ExecutorServiceProvider` as the max blocking timeout if it's non-negative. - Add a `TimeoutProcessor` helper class to update the left timeout after calling all methods that accept the timeout parameter. - Call `close` on all `ExecutorServiceProvider`s in `ClientImpl::shutdown` with 500ms timeout, which could be long enough. In addition, in `handleClose` method, call `shutdown` in another thread to avoid the deadlock. ### Verifying this change After applying this patch, the reproduce code in apache#13627 will pass the valgrind check. ``` ==3013== LEAK SUMMARY: ==3013== definitely lost: 0 bytes in 0 blocks ==3013== indirectly lost: 0 bytes in 0 blocks ==3013== possibly lost: 0 bytes in 0 blocks ``` (cherry picked from commit cd78f39) (cherry picked from commit 6d365c9)
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.
Fixes #13267
Motivation
Unlike Java client, the
Clientof C++ client has ashutdownmethodthat is responsible to execute the following steps:
shutdownon all internal producers and consumersWhen an executor is closed, it call
io_service::stop(), which makesthe event loop (
io_service::run()) in another thread return as soon aspossible. However, there is no wait operation. If a client failed to
create a producer or consumer, the
closemethod will callshutdownand close all executors immediately and exits the application. In this
case, the detached event loop thread might not exit ASAP, then valgrind
will detect the memory leak.
This memory leak can be avoided by sleeping for a while after
Client::closereturns or there are still other things to do afterthat. However, we should still adopt the semantics that after
Client::shutdownreturns, all event loop threads should be terminated.Modifications
closemethod ofExecutorServiceandExecutorServiceProvideras the max blocking timeout if it'snon-negative.
TimeoutProcessorhelper class to update the left timeout aftercalling all methods that accept the timeout parameter.
closeon allExecutorServiceProviders inClientImpl::shutdownwith 500ms timeout, which could be long enough.In addition, in
handleClosemethod, callshutdownin anotherthread to avoid the deadlock.
Verifying this change
After applying this patch, the reproduce code in #13267 will pass the
valgrind check.
Documentation
Check the box below or label this PR directly.
Need to update docs?
doc-required(Your PR needs to update docs and you will update later)
no-need-doc(Please explain why)
doc(Your PR contains doc changes)
doc-added(Docs have been already added)