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

Permute ThreadId values #110725

Closed
wants to merge 1 commit into from
Closed

Conversation

joboet
Copy link
Contributor

@joboet joboet commented Apr 23, 2023

As discussed on Zulip, users should never rely on the value of ThreadId, so apply a simple permutation when generating ids to make it appear random.

permute was ported from https://www.gkbrk.com/wiki/avalanche-diagram/ by @Pointerbender. It is not and does not need to be cryptographically secure.

@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 23, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-log-analyzer

This comment has been minimized.

@joshtriplett
Copy link
Member

I really don't think we should do this.

I personally continue to think we should expose the underlying OS thread ID value. But if we're not going to do that, and we're making up our own values, is there some fundamental reason we should do this?

@joboet
Copy link
Contributor Author

joboet commented Apr 23, 2023

I personally continue to think we should expose the underlying OS thread ID value.

That would be a breaking change, since ThreadIds are guaranteed not to be reused, but e.g. pthread_t does not have that guarantee.

But if we're not going to do that, and we're making up our own values, is there some fundamental reason we should do this?

If I understood the Zulip discussion correctly, it was proposed to do this to stop users from thinking that the id indicated the creating order of threads.

@the8472
Copy link
Member

the8472 commented Apr 23, 2023

I on the other hand believe that this doesn't go far enough and the permutation should be seeded with a random value, similar to hashmap iteration order.

But if we're not going to do that, and we're making up our own values, is there some fundamental reason we should do this?

The documentation says

The returned value is entirely opaque – only equality testing is stable. Note that it is not guaranteed which values new threads will return, and this may change across Rust versions.

Using a randomized permutation ensures that nobody relies on the value by accident. E.g. storing things by threadid in a btreemap and then relying on iteration order, writing tests against specific values or things like that.

@joshtriplett
Copy link
Member

I don't think we should be treating users as adversaries like this.

@the8472 the8472 added the I-libs-nominated Nominated for discussion during a libs team meeting. label Apr 23, 2023
@kennytm
Copy link
Member

kennytm commented Apr 23, 2023

-1.

This is totally overkill to introduce a permutation function just to obfuscate the implementation detail that the thread IDs are distributed sequentially. I don't support anything more complicated than XOR-ing with some magic number.

@joboet
Copy link
Contributor Author

joboet commented Apr 24, 2023

To be honest, the more I think about it, the less I like this. Relying on the exact value of an id is a bug, and permuting them won't help users fix it more easily. Bugs because of randomness are just as hard to debug as bugs relying on operation ordering.

I'm keeping this PR up, in case T-libs decides to do this. Feel free to close it otherwise.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 25, 2023

Looking at all the comments above, it looks like there is not much enthusiasm for this change.

Feel free to close it

Doing that.

@m-ou-se m-ou-se closed this Apr 25, 2023
@joboet joboet deleted the permute_threadid branch April 25, 2023 16:43
@dtolnay dtolnay removed the I-libs-nominated Nominated for discussion during a libs team meeting. label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants