Skip to content

fix: CompletionQueue::RunAsync is always async - #4448

Merged
coryan merged 2 commits into
googleapis:masterfrom
coryan:fix-completion-queue-async
Jul 1, 2020
Merged

fix: CompletionQueue::RunAsync is always async#4448
coryan merged 2 commits into
googleapis:masterfrom
coryan:fix-completion-queue-async

Conversation

@coryan

@coryan coryan commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

Prior to this change we sometimes ran the callable passed to
CompletionQueue::RunAsync() in the calling thread and not
in the thread pool, which was hard to reason about.

As part of these changes I also fixed the code to support
move-only callables, and added a test for it.

The code is "future proofed" to support a void() callable.
Which is interesting for a number of reasons (think "executors").

Fixes #4083


This change is Reviewable

Prior to this change we sometimes ran the callable passed to
`CompletionQueue::RunAsync()` in the calling thread and not
in the thread pool, which was hard to reason about.

As part of these changes I also fixed the code to support
move-only callables, and added a test for it.

The code is "future proofed" to support a `void()` callable.
Which is interesting for a number of reasons (think "executors").
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 30, 2020
@coryan
coryan marked this pull request as ready for review June 30, 2020 17:01
@coryan
coryan requested a review from a team June 30, 2020 17:01
Comment thread google/cloud/completion_queue.cc Outdated
@codecov

codecov Bot commented Jul 1, 2020

Copy link
Copy Markdown

Codecov Report

Merging #4448 into master will decrease coverage by 0.11%.
The diff coverage is 98.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4448      +/-   ##
==========================================
- Coverage   92.60%   92.49%   -0.12%     
==========================================
  Files         880      880              
  Lines       68262    68320      +58     
==========================================
- Hits        63215    63193      -22     
- Misses       5047     5127      +80     
Impacted Files Coverage Δ
google/cloud/completion_queue.cc 97.91% <95.00%> (-2.09%) ⬇️
google/cloud/completion_queue.h 76.47% <100.00%> (-11.03%) ⬇️
google/cloud/completion_queue_test.cc 95.65% <100.00%> (+0.71%) ⬆️
.../cloud/storage/oauth2/compute_engine_credentials.h 24.32% <0.00%> (-75.68%) ⬇️
google/cloud/spanner/testing/mock_spanner_stub.h 11.76% <0.00%> (-70.59%) ⬇️
google/cloud/iam_binding.h 50.00% <0.00%> (-50.00%) ⬇️
google/cloud/spanner/instance.h 50.00% <0.00%> (-50.00%) ⬇️
google/cloud/internal/pagination_range.h 40.74% <0.00%> (-44.45%) ⬇️
google/cloud/bigtable/internal/async_poll_op.h 35.13% <0.00%> (-35.14%) ⬇️
...gle/cloud/bigtable/internal/async_longrunning_op.h 61.11% <0.00%> (-33.34%) ⬇️
... and 66 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 90d36cb...aa37151. Read the comment docs.

@coryan
coryan merged commit 8fb6e1f into googleapis:master Jul 1, 2020
@coryan
coryan deleted the fix-completion-queue-async branch July 1, 2020 13:02
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.

Potential deadlocks in bigtable::MutationBatcher

3 participants