Skip to content

Fix TSAN json_run_localhost flake#13409

Merged
kpayson64 merged 1 commit intogrpc:masterfrom
kpayson64:fix_json_run_localhost
Nov 17, 2017
Merged

Fix TSAN json_run_localhost flake#13409
kpayson64 merged 1 commit intogrpc:masterfrom
kpayson64:fix_json_run_localhost

Conversation

@kpayson64
Copy link
Copy Markdown
Contributor

@kpayson64 kpayson64 commented Nov 15, 2017

Fixes #13178

@kpayson64 kpayson64 requested a review from dgquintas November 15, 2017 21:49
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 15, 2017 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 15, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 15, 2017 

[microbenchmarks] No significant performance differences

dgquintas
dgquintas reviewed Nov 16, 2017
View reviewed changes
test/cpp/qps/client_async.cc Outdated
Copy link
Copy Markdown
Contributor

@dgquintas dgquintas Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a sync operation?

Copy link
Copy Markdown
Contributor Author

@kpayson64 kpayson64 Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, the doc says this is "out of band".

FWIW, this is the same shutdown logic that has been used for a while. This just adds a shutdown check before the main loop, the issue was that things were getting shut down before we entered the main loop, and we didn't check first.

Copy link
Copy Markdown
Contributor

@vjpai vjpai Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Since this is just duplicated code from down below, can you take it out and make it a separate function or lambda or something?

Copy link
Copy Markdown
Contributor Author

@kpayson64 kpayson64 Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Copy Markdown
Contributor

@dgquintas dgquintas Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ask because it looks scary to delete the ctx right after an async op has been performed "on" it. But if this is kosher, LGTM

Copy link
Copy Markdown
Contributor

@vjpai vjpai Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dgq to answer your question: TryCancel just happens purely asynchronously without any CQ tag returning or anything, so it's fine to do a TryCancel and then delete the ctx.

@kpayson64 kpayson64 force-pushed the fix_json_run_localhost branch from 7d67f02 to 70a8762 Compare November 16, 2017 19:55
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 16, 2017 

[microbenchmarks] No significant performance differences

vjpai
vjpai reviewed Nov 16, 2017
View reviewed changes
test/cpp/qps/client_async.cc Outdated
Copy link
Copy Markdown
Contributor

@vjpai vjpai Nov 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullptr

Copy link
Copy Markdown
Contributor Author

@kpayson64 kpayson64 Nov 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

vjpai
vjpai suggested changes Nov 16, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@vjpai vjpai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Near LGTM, please just s/NULL/nullptr/

@kpayson64 kpayson64 force-pushed the fix_json_run_localhost branch from 70a8762 to 7cf8d72 Compare November 17, 2017 02:37
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Nov 17, 2017 

[microbenchmarks] No significant performance differences

vjpai
vjpai approved these changes Nov 17, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@vjpai vjpai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vjpai
Copy link
Copy Markdown
Contributor

vjpai commented Nov 17, 2017

For future reference, can you avoid squashing or rebasing while reviewers are active?

@kpayson64
Copy link
Copy Markdown
Contributor Author

kpayson64 commented Nov 17, 2017

@vjpai
Sorry about that, I'll squash after approval next time.

#13443
#13404
#13148

@kpayson64 kpayson64 merged commit 510fcb8 into grpc:master Nov 17, 2017
@vjpai vjpai mentioned this pull request Dec 4, 2017
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
@lock lock bot unassigned dgquintas Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@dgquintas dgquintas dgquintas left review comments

@vjpai vjpai vjpai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kpayson64 @grpc-testing @vjpai @dgquintas