Skip to content
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

[C++] Make some clean up methods thread safe #11762

Merged

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Aug 24, 2021

Fixes #11760

Motivation

BasicEndToEndTest.testLookupThrottling is flaky, from the logs of #11760 we can see the test crashed in ClientImpl::shutdown. The Connection closed log shows that ConnectionPool has been closed, so the problem is related to the close of three ExecutorServiceProviders of ClientImpl.

ExecutorServiceProvider::close is not thread safe because ExecutorServiceProvider::get might access the executors_ field in different threads and get uses a lock to guarantee thread-safety while close doesn't. In addition, some clean up methods including ClientImpl#shutdown and ExecutorService#close might be called twice or more, which is not necessary and has potential bugs.

Modifications

The main changes are:

  • Add a lock to guarantee thread-safety of ExecutorServiceProvider::close.
  • Use an atomic variable to make sure the logic in ExecutorService#close is called only once.
  • Avoid ConnectionPool#getConnectionAsync being called after close method is closed. In this case, return a future that is completed with ResultAlreadyClosed.
  • Add more debug logs.

In addition, this PR involves other changes. First, the master branch's source code cannot be compiled in my local env (macOS + Clang 12.0.0 + Boost 1.74), the error is:

/usr/local/include/boost/proto/expr.hpp:140:44: error: unknown warning group '-Wdeprecated-copy', ignored [-Werror,-Wunknown-warning-option]
            #pragma GCC diagnostic ignored "-Wdeprecated-copy"

I followed boostorg/proto#30 to fix this compilation error.

Another change is to fix the logs format, we can see the filename is always the same from logs in #11760. The reason is #11668 has changed logger() method from static to inline. It will make all C++'s translation units share the same and random chosen definition of logger(), but we need different definitions with different __FILE__ in different translation units. So this PR modified static back to inline and fix related compilation errors.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

@merlimat merlimat merged commit 098ba16 into apache:master Aug 25, 2021
@lhotari
Copy link
Member

lhotari commented Aug 25, 2021

Good work @BewareMyPower

@BewareMyPower BewareMyPower deleted the bewaremypower/cpp-executor-thread-safe branch August 25, 2021 08:02
codelipenghui pushed a commit that referenced this pull request Sep 18, 2021
* Make some close methods thread safe

* Restore shutdown() in ClientImpl's destructor and check whether connection pool is closed

(cherry picked from commit 098ba16)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Sep 18, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
* Make some close methods thread safe

* Restore shutdown() in ClientImpl's destructor and check whether connection pool is closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: C++ test BasicEndToEndTest.testLookupThrottling
4 participants