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

Tracking issue for std::thread::ThreadId #21507

Closed
aldanor opened this Issue Jan 22, 2015 · 22 comments

Comments

Projects
None yet
@aldanor
Copy link

aldanor commented Jan 22, 2015

Update description

Tracking issue for std::thread::ThreadId. Points to consider when stabilizing:

  • Implementation of a mutex + u64, unconditional panic if space is exhausted
  • Various traits on ThreadId, including Copy

Original report

Not sure if it can be considered as bug but it sure cripples the existing threading API (e.g., makes it harder for a thread to identify itself or store/pass its identity).

// it could also be nice if Thread was able to return a unique integer id() (or hash itself?..).

@sfackler sfackler added the A-libs label Jan 22, 2015

@murarth

This comment has been minimized.

Copy link
Contributor

murarth commented Aug 26, 2015

I'd also like to have Eq implemented for Thread. Would a PR be accepted to this effect or would it require an RFC?

@remram44

This comment has been minimized.

Copy link
Contributor

remram44 commented Oct 27, 2015

I'd very much like some kind of thread id, I'm pretty sure every Thread implementation around gives numeric ids (Python has a string name much like Rust but also offers Thread.ident, "a nonzero integer").

@frewsxcv

This comment has been minimized.

Copy link
Member

frewsxcv commented Mar 20, 2016

FYI, here's the associated issue in the RFCs repo: rust-lang/rfcs#1435

@alexcrichton alexcrichton changed the title PartialEq not implemented for std::thread::Thread Tracking issue for std::thread::ThreadId Oct 9, 2016

bors added a commit that referenced this issue Oct 10, 2016

Auto merge of #36341 - sagebind:thread_id, r=alexcrichton
Add ThreadId for comparing threads

This adds the capability to store and compare threads with the current calling thread via a new struct, `std::thread::ThreadId`. Addresses the need outlined in issue #21507.

This avoids the need to add any special checks to the existing thread structs and does not rely on the system to provide an identifier for a thread, since it seems that this approach is unreliable and undesirable. Instead, this simply uses a lazily-created, thread-local `usize` whose value is copied from a global atomic counter. The code should be simple enough that it should be as much reliable as the `#[thread_local]` attribute it uses (however much that is).

`ThreadId`s can be compared directly for equality and have copy semantics.

Also see these other attempts:
- #29457
- #29448
- #29447

And this in the RFC repo: rust-lang/rfcs#1435
@ticki

This comment has been minimized.

Copy link
Contributor

ticki commented Feb 3, 2017

I think it would be fitting to implement Hash, like std::mem::Discriminant, which doesn't expose an actual value either.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Apr 2, 2017

Just to chime in here, it would be really nice to have a proper Debug impl - I have a multi-threaded program I'm trying to debug and I'm trying to work out which threads are hitting a certain bit of code. Doing a println with current::id() would be really easy, but I can't and there isn't a nice way to get around this afaikt (will end up transmuting of course).

@frewsxcv

This comment has been minimized.

Copy link
Member

frewsxcv commented Apr 2, 2017

There's a PR open to implement Hash for ThreadId: #41008

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Apr 2, 2017

@nrc that seems reasonable to me. I asked the author of #41008 to add that derive as well.

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 12, 2017

Rollup merge of rust-lang#41008 - sagebind:thread_id, r=alexcrichton
Derive Hash for ThreadId + better example

Derive `Hash` for `ThreadId` (see comments in rust-lang#21507). Useful for making maps based on thread, e.g. `HashMap<ThreadId, ?>`. Also update example code for thread IDs to be more useful.

frewsxcv added a commit to frewsxcv/rust that referenced this issue Apr 12, 2017

Rollup merge of rust-lang#41008 - sagebind:thread_id, r=alexcrichton
Derive Hash for ThreadId + better example

Derive `Hash` for `ThreadId` (see comments in rust-lang#21507). Useful for making maps based on thread, e.g. `HashMap<ThreadId, ?>`. Also update example code for thread IDs to be more useful.

bors added a commit that referenced this issue Apr 12, 2017

Auto merge of #41008 - sagebind:thread_id, r=alexcrichton
Derive Hash for ThreadId + better example

Derive `Hash` for `ThreadId` (see comments in #21507). Useful for making maps based on thread, e.g. `HashMap<ThreadId, ?>`. Also update example code for thread IDs to be more useful.

bors added a commit that referenced this issue Apr 12, 2017

Auto merge of #41008 - sagebind:thread_id, r=alexcrichton
Derive Hash for ThreadId + better example

Derive `Hash` for `ThreadId` (see comments in #21507). Useful for making maps based on thread, e.g. `HashMap<ThreadId, ?>`. Also update example code for thread IDs to be more useful.

bors added a commit that referenced this issue Apr 12, 2017

Auto merge of #41008 - sagebind:thread_id, r=alexcrichton
Derive Hash for ThreadId + better example

Derive `Hash` for `ThreadId` (see comments in #21507). Useful for making maps based on thread, e.g. `HashMap<ThreadId, ?>`. Also update example code for thread IDs to be more useful.
@aturon

This comment has been minimized.

Copy link
Member

aturon commented May 11, 2017

@rust-lang/libs, thoughts on stabilization?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 11, 2017

@aturon some might say you could even canvas opinions through...

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented May 11, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented May 12, 2017

BTreeMap wants to know whether we could get an impl Ord for ThreadId. Are there any reasons not to have one?

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented May 12, 2017

Ord is uncomfortable to me. For instance, people might observe that the IDs sort in order of creation, but do we want to promise that?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 12, 2017

I'd personally go either way on Ord, I don't mind starting out conservatively and I also wouldn't mind adding it. If we do add it I'd at least want to clearly document "you really shouldn't rely on orderings between thread ids, just that you can order them to deduplicate"

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented May 16, 2017

I'm not super concerned about the Ord impl. We certainly don't want to promise that IDs sort in order of creation - TIDs on Unix systems wrap around pretty frequently.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented May 16, 2017

FWIW, in the current implementation it's a monotonic u64 counter, with no relationship to the OS TID at all. So it will in fact sort in the order of creation, or the order when first seen for threads created outside of Rust. But I still wouldn't want to promise this.

@dlight

This comment has been minimized.

Copy link

dlight commented May 16, 2017

If ThreadId is going to implement Ord, it probably makes sense to implement Hash too.

@sagebind

This comment has been minimized.

Copy link
Contributor

sagebind commented May 16, 2017

@dlight Currently, it already implements Hash.

I don't see an issue with implementing Ord, so long as we warn in the docs that the sort order doesn't indicate anything about the Threads they correspond to.

The only issue I see is if we change the implementation later down the road to use system handles, it would be difficult to provide a stable Ord implementation (e.g. there's no pthread_cmp(), only pthread_equal()).

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented May 16, 2017

We wouldn't be able to implement Hash for system handles anyway, so I think that ship has probably sailed.

@cuviper

This comment has been minimized.

Copy link
Member

cuviper commented May 16, 2017

System handles also aren't globally unique when threads stop and new ones start. Is that a property we want to guarantee about ThreadId?

@sagebind

This comment has been minimized.

Copy link
Contributor

sagebind commented May 16, 2017

We wouldn't be able to implement Hash for system handles anyway, so I think that ship has probably sailed.

That's a good point. I think that's why we chose not to use system handles in the first place: it makes ThreadId much harder to use and less useful.

@cuviper We don't currently make such a guarantee (which should reflect in the docs), but I think we should guarantee globally unique IDs. Assuming we don't plan on changing the IDs from being a custom integer. If the range of IDs becomes a problem, it is easy enough to increase the number of bits being used and maintain that guarantee.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented May 23, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jun 2, 2017

The final comment period is now complete.

sfackler added a commit to sfackler/rust that referenced this issue Jun 16, 2017

sfackler added a commit to sfackler/rust that referenced this issue Jun 19, 2017

sfackler added a commit to sfackler/rust that referenced this issue Jun 20, 2017

@bors bors closed this in dc411e3 Jun 28, 2017

brson added a commit to brson/rust that referenced this issue Jul 8, 2017

alexcrichton added a commit to brson/rust that referenced this issue Jul 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.