Skip to content

Replaced gpr_tls with C++ thread_local#20413

Closed
mhaidrygoog wants to merge 1 commit intogrpc:masterfrom
mhaidrygoog:use_thread_local
Closed

Replaced gpr_tls with C++ thread_local#20413
mhaidrygoog wants to merge 1 commit intogrpc:masterfrom
mhaidrygoog:use_thread_local

Conversation

@mhaidrygoog
Copy link
Copy Markdown
Contributor

now that we have access to it in core

@mhaidrygoog mhaidrygoog added area/core lang/core release notes: no Indicates if PR should not be in release notes labels Sep 30, 2019
@mhaidrygoog mhaidrygoog requested a review from vjpai September 30, 2019 21:38
@mhaidrygoog mhaidrygoog self-assigned this Sep 30, 2019
@veblush
Copy link
Copy Markdown
Contributor

veblush commented Oct 1, 2019

It's one of benefits that we can have with full C++11. It looks great! But we need to be careful because it doesn't work well on one of platforms. In this case, XCode 9.2 doesn't seem to be able to build it.

From a SO thread, XCode 8 and later should build this code so our CI using XCode 9.2 might have to handle this but it fails to build it. I think this is because of ios deployment target set by podspec.

  s.ios.deployment_target = '7.0'
  s.osx.deployment_target = '10.9'
  s.tvos.deployment_target = '10.0'

@mhaidrygoog Can you update it to 9.0 to see it's working?

@muxi Do you think it's feasible to bump it to 9.0 so that we can use thread_local in our code? I think iOS 9 is pretty old enough so that most of apple devices support it. (ref)

side-note: Another resource is worth reading: ThreadLocal.

@muxi
Copy link
Copy Markdown
Contributor

muxi commented Oct 1, 2019

No we can't bump it to 9.0. At the very least, Firestore supports 8.0 and depends on gRPC.

There could be other 3rd party users in the wild that support ever older versions

@veblush
Copy link
Copy Markdown
Contributor

veblush commented Oct 1, 2019

@muxi Thank you for the answer. Disappointing but we keep some workaround for at least Apple. Abseil also does the same thing for Apple because thread_local is not considered available.

@stale
Copy link
Copy Markdown

stale bot commented Mar 29, 2020

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core disposition/stale lang/core 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.

3 participants