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

runtime: fix Stacked Borrows violation in LocalOwnedTasks #5099

Merged
merged 2 commits into from Oct 12, 2022

Commits on Oct 12, 2022

  1. Copy the full SHA
    f408481 View commit details
    Browse the repository at this point in the history
  2. runtime: fix Stacked Borrows violation in LocalOwnedTasks

    ## Motivation
    
    It turns out that `LocalSet` has...never actually passed Miri. This is
    because all of the tests for `LocalSet` are defined as integration tests
    in `tokio/tests/task_local_set.rs`, rather than lib tests in
    `tokio/src`, and we never run Miri for integration tests.
    
    PR #5095 added a new test reproducing an unrelated bug in `LocalSet`,
    which had to be implemented as a lib test, as it must make internal
    assertions about the state of the `LocalSet` that cannot be tested with
    just the public API. This test failed under Miri, and it turns out that
    the reason for this is unrelated to the change in #5095 --- `LocalSet`
    uses the `LocalOwnedTasks` type, which turns out to contain a stacked
    borrows violation due to converting an `&Header` into an
    `NonNull<Header>` when removing a task from the `LocalOwnedTasks` list.
    
    ## Solution
    
    Fortunately, this was actually quite easy to fix. The non-`Local`
    version of `OwnedTasks` already uses `Task::header_ptr()` to get a
    `NonNull<Header>` in its version of `remove`, which avoids this issue.
    Therefore, the fix was as simple as updating `LocalOwnedTasks` to do the
    same.
    
    I've also added a very simple lib test in `src/task/local_set.rs` that
    just creates a `LocalSet` and spawns a single task on it under a
    current-thread runtime. This test fails under Miri without the fix in
    this PR: https://github.com/tokio-rs/tokio/actions/runs/3237072694/jobs/5303682530
    
    We should probably keep the test so that we're always testing at least
    the most trivial `LocalSet` usage under Miri...
    hawkw committed Oct 12, 2022
    Copy the full SHA
    346b7c0 View commit details
    Browse the repository at this point in the history