task: illumos/Solaris have thread-local weirdness #6777
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hawkw
force-pushed
the
eliza/keep-illumos-weird
branch
from
9768864
to
6c3e4a0
Compare
Darksonn
added
A-tokio
Darksonn
approved these changes
Aug 15, 2024
Comment on lines
-1163
to
+1166
| // FreeBSD has some weirdness around thread-local destruction. | ||
| // BSDs, illumos, and Solaris have some weirdness around thread-local | ||
| // destruction. | ||
| // TODO: remove this hack when thread id is cleaned up | ||
| #[cfg(not(any(target_os = "openbsd", target_os = "freebsd")))] | ||
| #[cfg(not(any( |
There was a problem hiding this comment.
Hmm, the actual code looks like this:
context::thread_id()
.map(|id| id == self.owner)
.unwrap_or(true),
here it's using map to handle the case where getting the thread local fails. Not sure why these platforms fail in some other way. Anyway, since we already need this for other platforms, it doesn't seem like an issue to add more platforms here.
There was a problem hiding this comment.
Yeah, if memory serves, the problem is actually that on BSDs (and apparently, illumos/Solaris), getting the thread local apparently doesn't fail as we would expect, and it instead gets a new value or something. I vaguely recall @carllerche trying to figure this out while working on #5179, and I think we couldn't really figure out what was going on there and kinda gave up...
## Motivation In `tokio::task::local`, there's a `LocalState::assert_called_from_owner_thread` function, which checks that the caller's thread ID matches that of the thread on which the `LocalSet` was created. This assertion is disabled on FreeBSD and OpenBSD, with a comment indicating that this is because those systems have "some weirdness around thread-local destruction". If memory serves, I believe the "weirdness" was related to the order in which thread-locals are destroyed relative to each other, or something along those lines. Upon running the tests on illumos, there appears to be [a similar panic][1] in the `task_local_set_in_thread_local` due to this assertion assertion while dropping a `LocalSet` which lives in a thread-local. This leads me to believe that illumos, and presumably Solaris, also exhibit the same "weirdness". This wouldn't be too surprising, given the shared heritage of those systems. ## Solution This commit adds `target_os="illumos"` and `target_os="solaris"` to the `cfg` attribute that disables the assertion on systems determined to have weirdness. This fixes the test panicking on illumos. In the future, it might be worthwhile to look into changign the assertion to only be disabled on weirdness-having systems _while dropping the `LocalSet`_, rather than all the time. That way, we can still check other accesses. But, I wanted to make the minimum change necessary to fix it here before messing around with that. [1]: https://buildomat.eng.oxide.computer/wg/0/details/01J592RB0JR203RGGN0RYHJHMY/CHieo1Uee7qzRVyp811YBl0MvXGO3i0QA9ezPaFWj6hf6P3P/01J592RSWCNX1MCFYGW74AHVH6#S1954
hawkw
force-pushed
the
eliza/keep-illumos-weird
branch
from
6c3e4a0
to
c0d199f
Compare
|
Looks like something is wrong with CI. You may have to push something to poke it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
In
tokio::task::local, there's aLocalState::assert_called_from_owner_threadfunction, which checksthat the caller's thread ID matches that of the thread on which the
LocalSetwas created. This assertion is disabled on FreeBSD andOpenBSD, with a comment indicating that this is because those systems
have "some weirdness around thread-local destruction". If memory serves,
I believe the "weirdness" was related to the order in which
thread-locals are destroyed relative to each other, or something along
those lines.
Upon running the tests on illumos, there appears to be a similar
panic in the
task_local_set_in_thread_localdue to this assertionassertion while dropping a
LocalSetwhich lives in a thread-local.This leads me to believe that illumos, and presumably Solaris, also
exhibit the same "weirdness". This wouldn't be too surprising, given the
shared heritage of those systems.
Solution
This commit adds
target_os="illumos"andtarget_os="solaris"to thecfgattribute that disables the assertion on systems determined tohave weirdness. This fixes the test panicking on illumos.
In the future, it might be worthwhile to look into changign the
assertion to only be disabled on weirdness-having systems while
dropping the
LocalSet, rather than all the time. That way, we canstill check other accesses. But, I wanted to make the minimum change
necessary to fix it here before messing around with that.
This change was cherry-picked from PR #6769.