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

[ios] rls_end2end_test fix #27831

Closed
wants to merge 1 commit into from
Closed

Conversation

dennycd
Copy link
Contributor

@dennycd dennycd commented Oct 27, 2021

Turns out when bazel run ios unit test, gtest's SetupTestSuite are invoked per unit test case, which would trigger assertion failure on duplicate registry on the 2nd and subsequent test case runs.

Alternative fix for calling ShutdownRegistry in test suite teardown doesn't seem to work as this seems to have reset internal states and causes all test case failures.

Follow up with grpc/grpc-ios#41 for investigating into why iOS is loading / running test this way.

@markdroth
Copy link
Member

I don't understand why this change would help. Even if the problem is that gtest is running SetUpTestSuite() per test case instead of once per test suite, then re-registering the LB policy after calling grpc_init() should be fine; it just means that we're doing all gRPC setup once per test case instead of once per test suite. The previous registry of the LB policy should not be present after calling grpc_shutdown_blocking() in TearDownTestSuite().

Why is the LB policy still registered after shutting down gRPC?

@markdroth
Copy link
Member

I think I'm going to make this particular problem go away by removing RLS from the gRPC build on mobile, since we've had unrelated complaints about the binary size increase with this policy. But it would be nice to still figure out what the actual root cause is here.

@markdroth
Copy link
Member

#27838 attempts to disable RLS on mobile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none lang/c++ platform/iOS release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants