Skip to content

[tls] Remove support for pthread tls#31040

Merged
ctiller merged 38 commits intogrpc:masterfrom
ctiller:remove-gpr-thread-local
Sep 23, 2022
Merged

[tls] Remove support for pthread tls#31040
ctiller merged 38 commits intogrpc:masterfrom
ctiller:remove-gpr-thread-local

Conversation

@ctiller
Copy link
Copy Markdown
Member

@ctiller ctiller commented Sep 17, 2022

Alternative to #31030, #31036

It's looking likely that our pthread TLS implementation is no longer necessary.
@dennycd is going to confirm.

Assuming that's the case, let's finally delete this gunk.

@ctiller
Copy link
Copy Markdown
Member Author

ctiller commented Sep 18, 2022

@dennycd I'm having a heck of a time sorting out ios deployment versions in this PR so I'll probably ask for some help with that piece assuming we can move forward with this.

@dennycd
Copy link
Copy Markdown
Contributor

dennycd commented Sep 20, 2022

Updating w/ deployment target change removed. TLS was introduced by Apple from XCode 8.x and up

@dennycd dennycd force-pushed the remove-gpr-thread-local branch 3 times, most recently from 54419d5 to 8655419 Compare September 20, 2022 23:15
@dennycd dennycd force-pushed the remove-gpr-thread-local branch from 8655419 to 5247f83 Compare September 20, 2022 23:51
@dennycd dennycd force-pushed the remove-gpr-thread-local branch from d90f8e4 to 065b6b2 Compare September 21, 2022 20:53
@dennycd dennycd force-pushed the remove-gpr-thread-local branch from b6b6b58 to bbf217e Compare September 21, 2022 21:54
@dennycd dennycd force-pushed the remove-gpr-thread-local branch from bbf217e to dfe9da9 Compare September 21, 2022 22:24
@dennycd
Copy link
Copy Markdown
Contributor

dennycd commented Sep 22, 2022

objc/iOS tests now all pass, a few summary notes below

  • grpc_objc_bazel_test: This is bazel-based run on newer MacOS 12.3 Monterey node with Xcode 13.3 (Clang 13.1.6). Our iOS configuration is (target device: iPhone 11, min deployment version: 9.0). This part is consistent w/ what we know about the TLS Apple support post Xcode 8/iOS 8.x. similarly for grpc_basictests_objc_ios

  • grpc_basictests_cpp_ios: Cocoapod based run on older MacOS 10.14 Mojave node with Xcode 11.3 (Clang 11.0). Our passing iOS configuration is (target device: iPhone 11, min deployment version: 10.0)

    • as tested, deployment versions below 10.0 appears not yet supporting TLS w/ thread_local

The discrepancy from # 2 likely come from running on MacOS Mojave node with older Xcode toolchain (XCode 11.3) which may not yet fully support TLS on all device targets.

Would recommend we upgrade grpc_basictests_cpp_ios to MacOS Monterey node in order to remove this configuration discrepancy (cl/475944915). ; )

Copy link
Copy Markdown
Contributor

@dennycd dennycd left a comment

Choose a reason for hiding this comment

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

iOS/ObjC part of this PR lgtm.

@ctiller ctiller merged commit f15ba1f into grpc:master Sep 23, 2022
@copybara-service copybara-service Bot added the imported Specifies if the PR has been imported to the internal repository label Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bloat/none imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants