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

docs: add note about directExecutor in example tests. #5672

Merged
merged 1 commit into from May 15, 2019

Conversation

rubensayshi
Copy link
Contributor

Since the HelloWorld tests are atm the "documentation" for how to do unit tests for grpc I think it would be good to add a note about the danger of the directExecutor(), particularly in bidirectional streaming.

I considered removing the directExecutor but it seems a common pattern in the grpc test suite and has it's merits so warning devs seems like a better solution.

related:

@ejona86 ejona86 requested a review from zhangkun83 May 9, 2019 17:04
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

We use a direct executor so the test is deterministic.

#2263 is unrelated; it's about using a fixed-size executor instead of our default cached thread pool executor, which can grow to any size. #3084 is a bug in our implementation, but it also requires a second thread to be in play. It looks like any streaming is likely impacted. I didn't realize #3084 was still open.

For now, maybe something along these lines:

directExecutor() makes it easier to have deterministic tests. However, if your implementation uses another thread and uses streaming it is better to use the default executor, to avoid hitting bug #3084 .

@rubensayshi rubensayshi force-pushed the unittest-direct-executor-note branch from 08bed14 to 3e44761 Compare May 15, 2019 10:48
@rubensayshi
Copy link
Contributor Author

@ejona86 okay, updated with your suggestion.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 15, 2019
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 15, 2019
@ejona86
Copy link
Member

ejona86 commented May 15, 2019

@zhangkun83, could you review as well?

@ejona86 ejona86 merged commit e6c8534 into grpc:master May 15, 2019
@ejona86
Copy link
Member

ejona86 commented May 15, 2019

@rubensayshi, thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants