amethyst / specs

Specs - Parallel ECS
https://amethyst.github.io/specs/
Apache License 2.0
2.49k stars 219 forks source link

Panic: Tried to fetch data, but it was already borrowed #660

Closed almindor closed 1 year ago

almindor commented 4 years ago

Description

I'm getting a runtime panic with: Tried to fetch data, but it was already borrowed

Meta

Rust version: 1.39.0 Specs version / commit: 0.15.1 Operating system: Arch Linux

Reproduction

Steps to reproduce the behavior: Issue seems to only happen if I call dispatch on two different Dispatchers under specific conditions when in between I modify a resource inside a separate code block.

I suspect this forces the mut reference to that resource to be dropped but somehow the second dispatcher run panics with this error.

Execution quasi-example:

{
    let mut state = world.fetch_mut::<State>();
    state.do_something_mutably();
}
dispatcher1.dispatch(&world);
world.maintain();

let mut state = world.fetch_mut::<State>(); // expecting a modified state from dispatcher1 run
state.do_something_else_mutably();
dispatcher2.dispatch(&world); // panics within here

Expected behavior

Both dispatchers should've executed fine.

Backtrace

thread '<unnamed>' panicked at 'Tried to fetch data, but it was already borrowed.
You can get the type name of the incorrectly borrowed data by enabling `shred`'s `nightly` feature.', .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/cell.rs:312:33
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.37/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:76
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:60
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1030
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1412
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:64
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:196
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:210
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:473
  11: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:380
  12: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:335
  13: shred::cell::TrustCell<T>::borrow_mut::{{closure}}
             at /.../.cargo/registry/src/github.com-1ecc6299db9ec823/specs-0.15.1/<::std::macros::panic macros>:9
  14: core::result::Result<T,E>::unwrap_or_else
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/result.rs:818
  15: shred::cell::TrustCell<T>::borrow_mut
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/cell.rs:311
  16: shred::world::World::try_fetch_mut::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/world/mod.rs:483
  17: core::option::Option<T>::map
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/option.rs:447
  18: shred::world::World::try_fetch_mut
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/world/mod.rs:482
  19: shred::world::World::fetch_mut
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/world/mod.rs:471
  20: <shred::world::data::Write<T,F> as shred::system::SystemData>::fetch
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/world/data.rs:118
  21: shred::system::impl_data::<impl shred::system::SystemData for (A, B, C, D, E)>::fetch
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/system.rs:372
  22: <T as shred::system::DynamicSystemData>::fetch
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/system.rs:266
  23: <T as shred::system::RunNow>::run_now
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/system.rs:139
  24: shred::dispatch::stage::Stage::execute::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/dispatch/stage.rs:110
  25: core::ops::function::impls::<impl core::ops::function::FnMut<A> for &F>::call_mut
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/ops/function.rs:245
  26: core::iter::traits::iterator::Iterator::for_each::call::{{closure}}
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/iter/traits/iterator.rs:613
  27: <core::slice::IterMut<T> as core::iter::traits::iterator::Iterator>::fold
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/slice/mod.rs:3213
  28: core::iter::traits::iterator::Iterator::for_each
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libcore/iter/traits/iterator.rs:616
  29: <rayon::iter::for_each::ForEachConsumer<F> as rayon::iter::plumbing::Folder<T>>::consume_iter
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/for_each.rs:55
  30: rayon::iter::plumbing::Producer::fold_with
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/plumbing/mod.rs:110
  31: rayon::iter::plumbing::bridge_producer_consumer::helper
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/plumbing/mod.rs:438
  32: rayon::iter::plumbing::bridge_producer_consumer::helper::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/plumbing/mod.rs:418
  33: rayon_core::join::join_context::call_a::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/join/mod.rs:125
  34: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panic.rs:315
  35: std::panicking::try::do_call
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panicking.rs:292
  36: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:80
  37: std::panicking::try
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panicking.rs:271
  38: std::panic::catch_unwind
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panic.rs:394
  39: rayon_core::unwind::halt_unwinding
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/unwind.rs:17
  40: rayon_core::join::join_context::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/join/mod.rs:146
  41: rayon_core::registry::in_worker
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:853
  42: rayon_core::join::join_context
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/join/mod.rs:133
  43: rayon::iter::plumbing::bridge_producer_consumer::helper
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/plumbing/mod.rs:416
  44: rayon::iter::plumbing::bridge_producer_consumer
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/plumbing/mod.rs:397
  45: <rayon::iter::plumbing::bridge::Callback<C> as rayon::iter::plumbing::ProducerCallback<I>>::callback
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/plumbing/mod.rs:373
  46: <rayon::slice::IterMut<T> as rayon::iter::IndexedParallelIterator>::with_producer
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/slice/mod.rs:708
  47: rayon::iter::plumbing::bridge
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/plumbing/mod.rs:357
  48: <rayon::slice::IterMut<T> as rayon::iter::ParallelIterator>::drive_unindexed
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/slice/mod.rs:684
  49: rayon::iter::for_each::for_each
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/for_each.rs:12
  50: rayon::iter::ParallelIterator::for_each
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-1.2.1/src/iter/mod.rs:360
  51: shred::dispatch::stage::Stage::execute
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/dispatch/stage.rs:108
  52: shred::dispatch::dispatcher::Dispatcher::dispatch_par::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/shred-0.9.4/src/dispatch/dispatcher.rs:89
  53: rayon_core::thread_pool::ThreadPool::install::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/thread_pool/mod.rs:132
  54: rayon_core::registry::Registry::in_worker_cold::{{closure}}::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:501
  55: <rayon_core::job::StackJob<L,F,R> as rayon_core::job::Job>::execute::call::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/job.rs:113
  56: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panic.rs:315
  57: std::panicking::try::do_call
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panicking.rs:292
  58: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:80
  59: std::panicking::try
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panicking.rs:271
  60: std::panic::catch_unwind
             at /rustc/4560ea788cb760f0a34127156c78e2552949f734/src/libstd/panic.rs:394
  61: rayon_core::unwind::halt_unwinding
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/unwind.rs:17
  62: <rayon_core::job::StackJob<L,F,R> as rayon_core::job::Job>::execute
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/job.rs:119
  63: rayon_core::job::JobRef::execute
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/job.rs:59
  64: rayon_core::registry::WorkerThread::execute
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:735
  65: rayon_core::registry::WorkerThread::wait_until_cold
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:719
  66: rayon_core::registry::WorkerThread::wait_until
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:693
  67: rayon_core::registry::main_loop
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:813
  68: rayon_core::registry::ThreadBuilder::run
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:58
  69: <rayon_core::registry::DefaultSpawn as rayon_core::registry::ThreadSpawn>::spawn::{{closure}}
             at .cargo/registry/src/github.com-1ecc6299db9ec823/rayon-core-1.6.1/src/registry.rs:103
azriel91 commented 4 years ago

Hiya, sorry for the late reply, but in the snippet, I suspect dispatcher2 contains a system that is accessing State mutably:

let mut state = world.fetch_mut::<State>(); // Get hold of a `Write<'_, State>`
state.do_something_else_mutably();
dispatcher2.dispatch(&world); // Trying to get another `Write'_, State>` causes the panic.

If you do this instead, it should work:

{
    let mut state = world.fetch_mut::<State>(); // Get hold of a `Write<'_, State>`
    state.do_something_else_mutably();

    // drop the `Write<'_, State>`
}
dispatcher2.dispatch(&world); // Now you can get a `Write'_, State>` without panicking

A potential improvement is to allow multiple Write's to be held, but only panic if both are dereferenced together. I haven't gotten familiar with the code to implement that yet.

Imberflur commented 1 year ago

A potential improvement is to allow multiple Write's to be held, but only panic if both are dereferenced together. I haven't gotten familiar with the code to implement that yet.

This would require being able to obtain a guard type from the Write that then dereferences to the actual value. Then it could be made to panic if the guard already exists when another guard is created. (there is no way to leverage dereferencing alone to detect conflicts since we can't detect when the reference stops being used) (right now Write basically acts as that guard so this would be adding another layer of types to defer the borrow that is currently held by Write).

I think doing this by default everywhere would, introduce additional complexity from the user's view point of view in that they now need to call a method to get the guard instance before that will dereference into the inner type.

Potentially, we could introduce an optional LazyWrite<'a, T> type that then has a method to get a Write<'a, T> (i.e. Write acts as the guard). However, I don't currently have any use case in mind where this would be useful and worth the complexity. If you have a compelling use case for such a feature, feel free to open an issue to discuss!