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

LocalSet and Runtime cannot be stored in thread_local at the same time #5162

Closed
Forsworns opened this issue Nov 3, 2022 · 5 comments · Fixed by #5179
Closed

LocalSet and Runtime cannot be stored in thread_local at the same time #5162

Forsworns opened this issue Nov 3, 2022 · 5 comments · Fixed by #5179
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-task Module: tokio/task

Comments

@Forsworns
Copy link

Forsworns commented Nov 3, 2022

Version

tokio 1.21.2

Platform
x86_64-unknown-linux-gnu

Description
It seems similar to #4973

I tried this code:

// src/main.rs
use tokio::runtime::Runtime;
use tokio::task::LocalSet;

thread_local! {
    pub static LOCAL_SET: TokioLocalSet = TokioLocalSet::new();
}

// the order here seems wrong, 
// have revised, see the following comments
pub struct TokioLocalSet {
    rt: Runtime,
    local: LocalSet,
}

impl TokioLocalSet {
    pub fn new() -> TokioLocalSet {
        TokioLocalSet {
            rt: tokio::runtime::Builder::new_current_thread()
                .enable_all()
                .build()
                .unwrap(),
            local: LocalSet::new(),
        }
    }

    async fn inner_method(&self) {
        self.local
            .run_until(async move {
                tokio::task::spawn_local(async {});
            })
            .await
    }

    pub fn method(&self) {
        self.rt.block_on(self.inner_method());
    }
}

fn main1(){
    // will panic
    LOCAL_SET.with(|f|{
        f.method();
    });
}

fn main2(){
    // work well
    let ls = TokioLocalSet::new();
    ls.method();
}

fn main3(){
    // work well
    let ls = TokioLocalSet::new();
    ls.method();
    LOCAL_SET.with(|f|{
        f.method();
    });
}
# Cargo.toml
[package]
name = "tokio_local_set"
version = "0.1.0"
edition = "2021"

[dependencies]
tokio = { version = "1.21.2", features = ["full"] }

I expected to see this happen: main1 works well.

Instead, this happened: main3 works well, main1 panics. And reported the same error as #4973

thread 'main' panicked at 'cannot access a Thread Local Storage value during or after destruction: AccessError'

Is this expected?

Here is the link to the playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e26d1f993906627a219d304e849cc70b

@Forsworns Forsworns added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Nov 3, 2022
@Darksonn Darksonn added the M-task Module: tokio/task label Nov 3, 2022
@Forsworns
Copy link
Author

Forsworns commented Nov 3, 2022

The stack trace is

stack backtrace:                                                                                                                
   0: rust_begin_unwind                                                                                                         
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/std/src/panicking.rs:575:5                              
   1: core::panicking::panic_fmt                                                                                                
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/panicking.rs:65:14                             
   2: core::result::unwrap_failed                                                                                               
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/result.rs:1791:5                               
   3: core::result::Result<T,E>::expect                                                                                         
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/result.rs:1070:23                              
   4: std::thread::local::LocalKey<T>::with                                                                                     
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/std/src/thread/local.rs:422:9                           
   5: tokio::runtime::context::budget                                                                                           
             at /root/tokio/tokio/src/runtime/context.rs:44:5                                                                   
   6: tokio::runtime::coop::with_budget
             at /root/tokio/tokio/src/runtime/coop.rs:93:5
   7: tokio::runtime::coop::budget
             at /root/tokio/tokio/src/runtime/coop.rs:70:5
   8: tokio::runtime::park::CachedParkThread::block_on
             at /root/tokio/tokio/src/runtime/park.rs:272:31
   9: tokio::runtime::enter::Enter::block_on
             at /root/tokio/tokio/src/runtime/enter.rs:152:13
  10: tokio::runtime::blocking::shutdown::Receiver::wait
             at /root/tokio/tokio/src/runtime/blocking/shutdown.rs:67:21
  11: tokio::runtime::blocking::pool::BlockingPool::shutdown
             at /root/tokio/tokio/src/runtime/blocking/pool.rs:213:12
  12: <tokio::runtime::blocking::pool::BlockingPool as core::ops::drop::Drop>::drop
             at /root/tokio/tokio/src/runtime/blocking/pool.rs:230:9
  13: core::ptr::drop_in_place<tokio::runtime::blocking::pool::BlockingPool>
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/ptr/mod.rs:490:1
  14: core::ptr::drop_in_place<tokio::runtime::runtime::Runtime> 
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/ptr/mod.rs:490:1
  15: core::ptr::drop_in_place<tokio_local_set::TokioLocalSet>
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/ptr/mod.rs:490:1

I thought it was due to the ordering of drop, so revised the struct to

pub struct TokioLocalSet {
    local: LocalSet,
    rt: Runtime,
}

It still panics, and the new stack trace is

stack backtrace:                                                                                                                
   0: rust_begin_unwind                                                                                                         
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/std/src/panicking.rs:575:5                              
   1: core::panicking::panic_fmt                                                                                                
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/panicking.rs:65:14                             
   2: core::result::unwrap_failed                                                                                               
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/result.rs:1791:5                               
   3: core::result::Result<T,E>::expect                                                                                         
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/result.rs:1070:23                              
   4: std::thread::local::LocalKey<T>::with                                                                                     
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/std/src/thread/local.rs:422:9                           
   5: tokio::task::local::<impl tokio::runtime::task::Schedule for alloc::sync::Arc<tokio::task::local::Shared>>::release       
             at /root/tokio/tokio/src/task/local.rs:991:9                                                                       
   6: tokio::runtime::task::harness::Harness<T,S>::release                                                                      
             at /root/tokio/tokio/src/runtime/task/harness.rs:334:29                                                            
   7: tokio::runtime::task::harness::Harness<T,S>::complete                                                                     
             at /root/tokio/tokio/src/runtime/task/harness.rs:320:27                                                            
   8: tokio::runtime::task::harness::Harness<T,S>::shutdown
             at /root/tokio/tokio/src/runtime/task/harness.rs:156:9
   9: tokio::runtime::task::raw::shutdown
             at /root/tokio/tokio/src/runtime/task/raw.rs:235:5
  10: tokio::runtime::task::raw::RawTask::shutdown
             at /root/tokio/tokio/src/runtime/task/raw.rs:168:18 
  11: tokio::runtime::task::Task<S>::shutdown
             at /root/tokio/tokio/src/runtime/task/mod.rs:376:9
  12: tokio::runtime::task::list::LocalOwnedTasks<S>::close_and_shutdown_all
             at /root/tokio/tokio/src/runtime/task/list.rs:227:13
  13: <tokio::task::local::LocalSet as core::ops::drop::Drop>::drop::{{closure}}
             at /root/tokio/tokio/src/task/local.rs:823:13
  14: tokio::task::local::LocalSet::with_if_possible
             at /root/tokio/tokio/src/task/local.rs:705:35
  15: <tokio::task::local::LocalSet as core::ops::drop::Drop>::drop
             at /root/tokio/tokio/src/task/local.rs:820:9
  16: core::ptr::drop_in_place<tokio::task::local::LocalSet>
             at /rustc/edf0182213a9e30982eb34f3925ddc4cf5ed3471/library/core/src/ptr/mod.rs:490:1

@Darksonn
Copy link
Contributor

Darksonn commented Nov 3, 2022

It seems like this is because the LocalSet has tasks in it, and not because of the runtime being stored together with it.

@Forsworns
Copy link
Author

Thanks, I once thought it's due to some collisions between their deconstruction in TLS. You can revise the title if found a better one.

You are right, according to the stack trace, the tasks in LocalSet are releasing by the self.context.owned.close_and_shutdown_all() in its Drop implementation, while the LocalData CURRENT in TLS has not existed...

@Forsworns
Copy link
Author

I think I got a workaround, this works well.

use std::cell::Cell;
use tokio::runtime::Runtime;
use tokio::task::LocalSet;

thread_local! {
    pub static LOCAL_SET: Cell<Option<TokioLocalSet>> = Cell::new(None);
}

pub struct TokioLocalSet {
    ls: LocalSet,
    rt: Runtime,
}

impl TokioLocalSet {
    pub fn new() -> TokioLocalSet {
        TokioLocalSet {
            ls: LocalSet::new(),
            rt: tokio::runtime::Builder::new_current_thread()
                .enable_all()
                .build()
                .unwrap(),
        }
    }

    async fn inner_method(&self) {
        self.ls
            .run_until(async move {
                tokio::task::spawn_local(async {});
                println!("task called");
            })
            .await
    }

    pub fn method(&self) {
        self.rt.block_on(self.inner_method());
    }
}

fn main() {
    LOCAL_SET.with(|f| {
        f.set(Some(TokioLocalSet::new()));
    });
    LOCAL_SET.with(|f| {
        let ls = f.take().unwrap();
        ls.method();
        f.set(Some(ls));
    });
    LOCAL_SET.with(|f| {
        let ls = f.take().unwrap();
        ls.method();
        f.set(Some(ls));
    });
    LOCAL_SET.with(|f| {
        f.take().unwrap();
    });
}

@Darksonn
Copy link
Contributor

Darksonn commented Nov 4, 2022

We should still fix this, of course.

carllerche added a commit that referenced this issue Nov 10, 2022
`LocalSet` cleans up any tasks that have not yet been completed when it is
dropped. Previously, this cleanup process required access to a thread-local.
Suppose a `LocalSet` is stored in a thread-local itself. In that case, when it is
dropped, there is no guarantee the drop implementation will be able to
access the internal `LocalSet` thread-local as it may already have been
destroyed.

The internal `LocalSet` thread local is mainly used to avoid writing unsafe
code. All `LocalState` that cannot be moved across threads is stored in the
thread-local and accessed on demand.

This patch moves this local-only state into the `LocalSet`'s "shared" struct.
Because this struct *is* `Send`, the local-only state is stored in `UnsafeCell`,
and callers must ensure not to touch it from other threads.

A debug assertion is added to enforce this requirement in tests.

Fixes #5162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-task Module: tokio/task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants