Add mutex to protect exchange receiver's async client #5008

yibin87 · 2022-05-26T10:56:50Z

What problem does this PR solve?

Issue Number: close #4977

Problem Summary:

What is changed and how it works?

For ExchangeReceiver's async client mode, AsyncRequestHandler instance is used to handle async client request and rsp. And it is shared between grpc and reactor thread. However, when handler is creating, there is a little chance that grpc thread get the instance before the creation is completed. So there is chance that #4977 would happen.
Now this is noticed only when makeAsyncReader failed and retried.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot · 2022-05-26T10:56:51Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

LittleFall
windtalker

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: yibin <huyibin@pingcap.com>

fuzhe1989 · 2022-05-26T11:42:49Z

dbms/src/Flash/Mpp/ExchangeReceiver.cpp

@@ -393,10 +400,10 @@ void ExchangeReceiverBase<RPCContext>::reactor(const std::vector<Request> & asyn
    MPMCQueue<AsyncHandler *> ready_requests(alive_async_connections * 2);
    std::vector<AsyncHandler *> waiting_for_retry_requests;

-    std::vector<AsyncRequestHandler<RPCContext>> handlers;
+    std::vector<std::unique_ptr<AsyncHandler>> handlers;


I can't get what would happen without this change, could you tell me something?

In AsyncGrpcExchangePacketReader‘s init function, reader = cluster->rpc_client->sendStreamRequestAsync(xxx), the right part can execute first, and grpc thread sees the reader.
I start from the issue's core down stack, so this suspicous one could be the problem. And I can't tell a whole story about what happend either. Just this seems a suspicous one. And fixed binary doesn't core down.

yes that's a great job and I've got what happened about this bug.

The only remaining question is why you change the element type of handlers from a value type to a unique_ptr.

It's ok, but why?

mutex field is not support for copy constructor...

windtalker

LGTM

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 · 2022-05-26T12:09:04Z

/merge

ti-chi-bot · 2022-05-26T12:09:04Z

@yibin87: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-05-26T12:09:06Z

This pull request has been accepted and is ready to merge.

Commit hash: 606b704

ti-chi-bot · 2022-05-26T13:12:57Z

@yibin87: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

windtalker · 2022-05-26T13:40:35Z

/run-unit-test

yibin87 · 2022-05-26T14:14:27Z

/run-unit-test

sre-bot · 2022-05-26T14:25:17Z

Coverage for changed files

Filename                            Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Flash/Mpp/ExchangeReceiver.cpp          320               307     4.06%          32                27    15.62%         486               453     6.79%         192               188     2.08%
Functions/FunctionsDateTime.cpp          68                31    54.41%          14                 3    78.57%         197                58    70.56%          48                25    47.92%
Functions/FunctionsDateTime.h          1061               628    40.81%         328               211    35.67%        2005              1196    40.35%         516               327    36.63%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                  1449               966    33.33%         374               241    35.56%        2688              1707    36.50%         756               540    28.57%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18277      9786             46.46%    204784  98120        52.09%

full coverage report (for internal network access only)

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-05-26T14:26:41Z

In response to a cherrypick label: new pull request created: #5009.

ti-chi-bot · 2022-05-26T14:27:19Z

In response to a cherrypick label: new pull request created: #5010.

close #4977

yibin87 added 3 commits May 26, 2022 17:46

Add to_seconds support for tiflash

4493101

Signed-off-by: yibin <huyibin@pingcap.com>

Fix format issue

bdbd30a

Signed-off-by: yibin <huyibin@pingcap.com>

Add mutex lock to protect async reciever async reader

2222336

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 26, 2022

Fix a rebase issue

20d3c48

Signed-off-by: yibin <huyibin@pingcap.com>

yibin87 requested review from windtalker, fzhedu and LittleFall May 26, 2022 11:00

Change raw pointer to unique ptr

cbc998b

Signed-off-by: yibin <huyibin@pingcap.com>

LittleFall approved these changes May 26, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 26, 2022

fuzhe1989 reviewed May 26, 2022

View reviewed changes

yibin87 requested a review from fuzhe1989 May 26, 2022 11:54

windtalker approved these changes May 26, 2022

View reviewed changes

Fix format issue

606b704

Signed-off-by: yibin <huyibin@pingcap.com>

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 26, 2022

Merge branch 'master' into dev_feature_branch

240dcd7

yibin87 merged commit f10b6d2 into pingcap:master May 26, 2022

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 26, 2022

This is an automated cherry-pick of pingcap#5008

1e8677c

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 26, 2022

Add mutex to protect exchange receiver's async client (#5008) #5009

Closed

12 tasks

ti-chi-bot mentioned this pull request May 26, 2022

Add mutex to protect exchange receiver's async client (#5008) #5010

Merged

12 tasks

ti-chi-bot added a commit that referenced this pull request May 27, 2022

Add mutex to protect exchange receiver's async client (#5008) (#5010)

b34873a

close #4977

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mutex to protect exchange receiver's async client #5008

Add mutex to protect exchange receiver's async client #5008

yibin87 commented May 26, 2022

ti-chi-bot commented May 26, 2022 •

edited

Loading

fuzhe1989 May 26, 2022

yibin87 May 26, 2022 •

edited

Loading

fuzhe1989 May 26, 2022

yibin87 May 26, 2022

windtalker left a comment

yibin87 commented May 26, 2022

ti-chi-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022

windtalker commented May 26, 2022

yibin87 commented May 26, 2022

sre-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022

Add mutex to protect exchange receiver's async client #5008

Add mutex to protect exchange receiver's async client #5008

Conversation

yibin87 commented May 26, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented May 26, 2022 • edited Loading

fuzhe1989 May 26, 2022

Choose a reason for hiding this comment

yibin87 May 26, 2022 • edited Loading

Choose a reason for hiding this comment

fuzhe1989 May 26, 2022

Choose a reason for hiding this comment

yibin87 May 26, 2022

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

yibin87 commented May 26, 2022

ti-chi-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022

windtalker commented May 26, 2022

yibin87 commented May 26, 2022

sre-bot commented May 26, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022

ti-chi-bot commented May 26, 2022 •

edited

Loading

yibin87 May 26, 2022 •

edited

Loading