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

fix: allow DefaulBackgroundThreads to self-destroy #5681

Closed
wants to merge 1 commit into from

Conversation

dopiera
Copy link
Contributor

@dopiera dopiera commented Jan 6, 2021

The last reference to AutomaticallyCreatedBackgroundThreads may be in
the CompletionQueue owned by itself. This means that
~AutomaticallyCreatedBackgroundThreads() will be executed in the
thread belonging to it. Before this PR this resulted in a deadlock. This
PR fixes it.


This change is Reviewable

The last reference to `AutomaticallyCreatedBackgroundThreads` may be in
the `CompletionQueue` owned by itself. This means that
`~AutomaticallyCreatedBackgroundThreads()` will be executed in the
thread belonging to it. Before this PR this resulted in a deadlock. This
PR fixes it.
@dopiera dopiera requested a review from a team as a code owner January 6, 2021 14:06
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 6, 2021
@coryan
Copy link
Contributor

coryan commented Jan 6, 2021

When we have ran into this problem before we solved it by separating the ownership of the thread pool from the users of the CQ. For example, in pubsub we do this:

class ContainingPublisherConnection : public PublisherConnection {

In spanner we did this:

googleapis/google-cloud-cpp-spanner#1397

@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #5681 (088f5a3) into master (8275f58) will decrease coverage by 0.02%.
The diff coverage is 96.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5681      +/-   ##
==========================================
- Coverage   95.44%   95.42%   -0.03%     
==========================================
  Files        1106     1106              
  Lines      100378   100727     +349     
==========================================
+ Hits        95807    96116     +309     
- Misses       4571     4611      +40     
Impacted Files Coverage Δ
google/cloud/internal/background_threads_impl.h 100.00% <ø> (ø)
google/cloud/internal/background_threads_impl.cc 92.59% <80.00%> (-7.41%) ⬇️
google/cloud/bigtable/internal/common_client.cc 93.75% <94.44%> (+2.57%) ⬆️
google/cloud/bigtable/internal/common_client.h 96.82% <100.00%> (+0.15%) ⬆️
...ogle/cloud/bigtable/internal/common_client_test.cc 100.00% <100.00%> (ø)
...le/cloud/internal/default_completion_queue_impl.cc 96.98% <100.00%> (+0.21%) ⬆️
google/cloud/internal/async_connection_ready.h 50.00% <0.00%> (-50.00%) ⬇️
...e/cloud/storage/tests/key_file_integration_test.cc 91.52% <0.00%> (-2.71%) ⬇️
...oud/storage/tests/curl_request_integration_test.cc 96.57% <0.00%> (-2.50%) ⬇️
...on_tests/rpc_failure_threshold_integration_test.cc 86.59% <0.00%> (-2.07%) ⬇️
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59d0899...088f5a3. Read the comment docs.

@coryan
Copy link
Contributor

coryan commented Jan 14, 2021

Closing for now, we can reopen later.

@coryan coryan closed this Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants