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

thread_local: thread safety follow ups #13313

Open
mattklein123 opened this issue Sep 29, 2020 · 1 comment
Open

thread_local: thread safety follow ups #13313

mattklein123 opened this issue Sep 29, 2020 · 1 comment
Assignees

Comments

@mattklein123
Copy link
Member

A couple of follow ups to #12833:

  1. Try to make the API a bit cleaner with templating: tls: simplify implementation and fix one class of crashing bug #12833 (comment)
  2. Build clang-tidy plugin to check for any TLS functions capturing "this", and fix all current examples. (There are several that capture "this" in the initial set() call, and some that have been "fixed" to use shared_from_this and weak_ptr.)
@jmarantz
Copy link
Contributor

jmarantz commented Sep 30, 2020

Just to avoid the indirection I'll paste my referenced comment here with context:

to improve compile-time type safety, as well as less cumbersome usage, can we consider 2 changes:

  1. Templatize the usage of slotes, so I put the fact that this is slotting a TlsCache into my declaration: ThreadLocal::SlotPtr<TlsCache> and then, for me, asType() is not needed.
  2. Pass in object here as a non-const reference, so I can update if I like, but don't have to return it or declare the return-value.

Then one of the call-sites in source/common/stats/thread_local_store.cc changes from

    tls_->runOnAllThreads(
        [](ThreadLocal::ThreadLocalObjectSharedPtr object)
            -> ThreadLocal::ThreadLocalObjectSharedPtr {
          for (const auto& id_hist : object->asType<TlsCache>().tls_histogram_cache_) {
            const TlsHistogramSharedPtr& tls_hist = id_hist.second;
            tls_hist->beginMerge();
          }
          return object;
        },
        [this, merge_complete_cb]() -> void { mergeInternal(merge_complete_cb); });

to

  tls_->runOnAllThreads(
      [](TlsCache& tls_cache) {
        for (const auto& id_hist : tls_cache.tls_histogram_cache_) {
          const TlsHistogramSharedPtr& tls_hist = id_hist.second;
          tls_hist->beginMerge();
        }
      },
      [this, merge_complete_cb]() -> void { mergeInternal(merge_complete_cb); });

I think the Slot mechanism itself doesn't to change at all to accommodate this templating layer. It can do what it does now, and the templating API can be a thin header-only layer above.

@jmarantz jmarantz self-assigned this Oct 28, 2020
@mattklein123 mattklein123 removed their assignment Oct 29, 2020
jmarantz added a commit that referenced this issue Oct 29, 2020
Commit Message: Adds a new TypedSlot API, where the slot data is strongly typed. Also removes an unused capability to replace slot data with a new instance via the callbacks in runOnAllThreads.

Use this new API rather than the untyped Slot API for one of the sites (stats thread_local_store.cc).

This is a partial fix for #13313
Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants