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

std: Add a new top-level thread_local module #19094

Closed

Conversation

alexcrichton
Copy link
Member

This commit removes the std::local_data module in favor of a new std::thread_local
module providing thread local storage. The module provides two variants of TLS:
one which owns its contents and one which is based on scoped references. Each
implementation has pros and cons listed in the documentation.

Both flavors have accessors through a function called with which yield a
reference to a closure provided. Both flavors also panic if a reference cannot
be yielded and provide a function to test whether an access would panic or not.
This is an implementation of RFC 461 and full details can be found in
that RFC.

This is a breaking change due to the removal of the std::local_data module.
All users can migrate to the new tls system like so:

thread_local!(static FOO: Rc<RefCell<Option<T>>> = Rc::new(RefCell::new(None)))

The old local_data module inherently contained the Rc<RefCell<Option<T>>> as
an implementation detail which must now be explicitly stated by users.

[breaking-change]

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@alexcrichton
Copy link
Member Author

cc @aturon, @nikomatsakis, you may be interested in the API decisions here (everything panics)

@oli-obk
Copy link
Contributor

oli-obk commented Nov 19, 2014

I find "tls" to be a little ambiguous as a name, since it's already much in use for transport layer security.
Searching for problems will be quite troublesome, as the two need to be kept apart

/* Common traits */

pub mod error;
pub mod num;

/* Runtime and platform support */

pub mod tls;
Copy link
Contributor

Choose a reason for hiding this comment

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

You ordered this alphabetically, except for tls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly this is due to how macros work, some of the modules below need the tls macro which requires it to be defined first.

@alexcrichton alexcrichton force-pushed the rm-std-local-data branch 2 times, most recently from 50e714b to 300f350 Compare November 19, 2014 17:49
@frewsxcv
Copy link
Member

I'd also vote against utilizing the acronym tls, seems like it will only cause confusion, despite appearing attractively concise

@@ -213,7 +215,9 @@ pub const WARN: u32 = 2;
/// Error log level
pub const ERROR: u32 = 1;

local_data_key!(local_logger: Box<Logger + Send>)
tls!(static LOCAL_LOGGER: RefCell<Option<Box<Logger + Send>>> = {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's worth providing a newtype abstraction around RefCell<Option<T>> with a flattenend API; seems to come up from time to time.

@aturon
Copy link
Member

aturon commented Nov 21, 2014

OK, I've read over the entire implementation, pretty carefully. Great work!

I'm happy with the panic semantics here.

I agree with what others have mentioned about tls being a bit problematic, as we discussed on IRC. I don't care much what it's changed to (tld would be fine by me).

The module organization here strongly implies that the owned variant is the primary/default one, which is probably true, but we may want to revisit that before final stabilization.

Other than the tls naming issue, r=me

@alexcrichton alexcrichton force-pushed the rm-std-local-data branch 2 times, most recently from 1fb6f9e to 56a9f32 Compare November 21, 2014 17:06
@alexcrichton alexcrichton changed the title std: Add a new top-level tls module std: Add a new top-level thread_local module Nov 21, 2014
@alexcrichton
Copy link
Member Author

I've renamed every mention of "tls" to "thread_local"

@seanmonstar
Copy link
Contributor

Is there a way forward for the liblog to copy or share the logger with child tasks with this design?

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 22, 2014
This commit removes the `std::local_data` module in favor of a new `std::thread_local`
module providing thread local storage. The module provides two variants of TLS:
one which owns its contents and one which is based on scoped references. Each
implementation has pros and cons listed in the documentation.

Both flavors have accessors through a function called `with` which yield a
reference to a closure provided. Both flavors also panic if a reference cannot
be yielded and provide a function to test whether an access would panic or not.
This is an implementation of [RFC 461][rfc] and full details can be found in
that RFC.

This is a breaking change due to the removal of the `std::local_data` module.
All users can migrate to the new tls system like so:

    thread_local!(static FOO: Rc<RefCell<Option<T>>> = Rc::new(RefCell::new(None)))

The old `local_data` module inherently contained the `Rc<RefCell<Option<T>>>` as
an implementation detail which must now be explicitly stated by users.

[rfc]: rust-lang/rfcs#461
[breaking-change]
@alexcrichton
Copy link
Member Author

@seanmonstar probably not, but that's more of a problem for liblog than the inherent design of this module itself. The purpose of this module is to expose the system primitives available with a safe interface.

bors added a commit that referenced this pull request Nov 22, 2014
This commit removes the `std::local_data` module in favor of a new `std::thread_local`
module providing thread local storage. The module provides two variants of TLS:
one which owns its contents and one which is based on scoped references. Each
implementation has pros and cons listed in the documentation.

Both flavors have accessors through a function called `with` which yield a
reference to a closure provided. Both flavors also panic if a reference cannot
be yielded and provide a function to test whether an access would panic or not.
This is an implementation of [RFC 461][rfc] and full details can be found in
that RFC.

This is a breaking change due to the removal of the `std::local_data` module.
All users can migrate to the new tls system like so:

    thread_local!(static FOO: Rc<RefCell<Option<T>>> = Rc::new(RefCell::new(None)))

The old `local_data` module inherently contained the `Rc<RefCell<Option<T>>>` as
an implementation detail which must now be explicitly stated by users.

[rfc]: rust-lang/rfcs#461
[breaking-change]
This commit removes the `std::local_data` module in favor of a new
`std::thread_local` module providing thread local storage. The module provides
two variants of TLS: one which owns its contents and one which is based on
scoped references. Each implementation has pros and cons listed in the
documentation.

Both flavors have accessors through a function called `with` which yield a
reference to a closure provided. Both flavors also panic if a reference cannot
be yielded and provide a function to test whether an access would panic or not.
This is an implementation of [RFC 461][rfc] and full details can be found in
that RFC.

This is a breaking change due to the removal of the `std::local_data` module.
All users can migrate to the new thread local system like so:

    thread_local!(static FOO: Rc<RefCell<Option<T>>> = Rc::new(RefCell::new(None)))

The old `local_data` module inherently contained the `Rc<RefCell<Option<T>>>` as
an implementation detail which must now be explicitly stated by users.

[rfc]: rust-lang/rfcs#461
[breaking-change]
bors added a commit that referenced this pull request Nov 24, 2014
This commit removes the `std::local_data` module in favor of a new `std::thread_local`
module providing thread local storage. The module provides two variants of TLS:
one which owns its contents and one which is based on scoped references. Each
implementation has pros and cons listed in the documentation.

Both flavors have accessors through a function called `with` which yield a
reference to a closure provided. Both flavors also panic if a reference cannot
be yielded and provide a function to test whether an access would panic or not.
This is an implementation of [RFC 461][rfc] and full details can be found in
that RFC.

This is a breaking change due to the removal of the `std::local_data` module.
All users can migrate to the new tls system like so:

    thread_local!(static FOO: Rc<RefCell<Option<T>>> = Rc::new(RefCell::new(None)))

The old `local_data` module inherently contained the `Rc<RefCell<Option<T>>>` as
an implementation detail which must now be explicitly stated by users.

[rfc]: rust-lang/rfcs#461
[breaking-change]
@bors bors closed this Nov 24, 2014
@alexcrichton alexcrichton deleted the rm-std-local-data branch December 28, 2014 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants