Add logic to work around buggy Android NDKs#13173
Merged
nicolasnoble merged 19 commits intogrpc:masterfrom Nov 20, 2017
Merged
Conversation
Old version of the Android NDK have linker issues with thread local support android/ndk#8 and isn't actually fixed until r12b https://developer.android.com/ndk/downloads/revision_history.html. ABSL's config.h is being updated to catch this case and having gRPC rely on that will make sure it also gets the fix (rather than repeating a somewhat lengthy macro). Since gRPC already has a dependency on ABSL, I figured this wouldn't be an issue.
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
Contributor
Author
|
signed linuxfoundation CLA |
Use pthreads on posix instead of relying on thread local support
nicolasnoble
reviewed
Nov 1, 2017
src/core/lib/support/cpu_posix.cc
Outdated
| unsigned int *thread_id = | ||
| static_cast<unsigned int *>(pthread_getspecific(thread_id_key)); | ||
| if (thread_id == nullptr) { | ||
| thread_id = new unsigned int(thread_counter++); |
Contributor
There was a problem hiding this comment.
Sorry, we can't use new and delete into the core: https://github.com/grpc/grpc/blob/master/doc/core/moving-to-c%2B%2B.md
Please change to using gpr_malloc and gpr_free instead.
Contributor
Author
|
Updated with comments from @nicolasnoble |
|
|
|
Copy over Absl macro to deal with C++/C differences.
Contributor
Author
|
Changed to fix C++ vs C issues. |
|
|
Contributor
Author
|
Updated to not depend on c++ runtime |
|
|
|
|
|
|
Contributor
|
I'm not sure if https://sponge.corp.google.com/target?id=69000522-fcae-4512-b985-703153bcaca0&target=github/grpc/c_macos_opt_native&searchFor=&show=FAILED&sortBy=STATUS isn't a new failure, potentially caused by this... |
Contributor
Author
|
That failure doesn't seem to go through the callpaths in this CL (or rely on having too many pthread_key's created). I'm looking into the best way to run this test on my end |
Contributor
Author
|
Got this test working locally and it does seem to related to this change. Debugging now. |
Don't delete the key in the pthread_key_create destructor. The key isn't specific to instances of values.
Contributor
Author
|
Ok, Fix should be uploaded now. I'm hoping this is the last fix needed |
|
|
|
nicolasnoble
approved these changes
Nov 20, 2017
Contributor
|
All of the test failures are now known flakes, merging. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Old version of the Android NDK have linker issues with thread local support android/ndk#8 and isn't actually fixed until r12b https://developer.android.com/ndk/downloads/revision_history.html. ABSL's config.h is being updated to catch this case and having gRPC rely on that will make sure it also gets the fix (rather than repeating a somewhat lengthy macro).
Since gRPC already has a dependency on ABSL, I figured this wouldn't be an issue.