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

Make {Mutex, Notify, OnceCell, RwLock, Semaphore}::const_new always available #5885

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Jul 21, 2023

Motivation

MSRV is bumped to 1.63 in #5887 , nowMutex::new is usable in const context, so these functions should now be always available.

Solution

This patch makes these function always available.

Also use assert! in const function to ensure correctness instead of silently truncating the value and remove use of cfg tokio_no_const_mutex_new.

This PR might resolve #4623

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Jul 21, 2023
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jul 21, 2023

Why not make Mutex::new and others const on rust-version >= 1.63?

Unfortunately they can't since with cfg(all(tokio_unstable, feature = "tracing")) Mutex::new will log using tracing::trace! and it also constructs a span using tracing::trace_span!.

From #4623 (comment)

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jul 21, 2023

I see #5877 tries to bump msrv to 1.63 and it will eliminate the use of build.rs and make const_new always available.
If that is desired, I can close this PR and open another one to bump msrv and removes build.rs.

Or merging this PR first then talk about bumping msrv later, I'm fine with either.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would prefer to raise the MSRV, but we will have to hear from other maintainers on that.

tokio/src/sync/mutex.rs Outdated Show resolved Hide resolved
@NobodyXu NobodyXu requested a review from taiki-e July 21, 2023 08:27
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-sync Module: tokio/sync labels Jul 21, 2023
@carllerche
Copy link
Member

We can raise the MSRV to 1.63 and remove the trace call from Mutex::new (tracing is unstable).

@github-actions github-actions bot added the R-loom-sync Run loom sync tests on this PR label Jul 22, 2023
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Jul 22, 2023

Somehow rebasing this against main caused CI to fail:

thread 'tokio-runtime-worker' panicked at 'explicit panic', tokio/src/runtime/scheduler/multi_thread_alt/queue.rs:149:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/panicking.rs:67:14
   2: core::panicking::panic
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/panicking.rs:117:5
   3: tokio::runtime::scheduler::multi_thread_alt::queue::Local<T>::push_back
             at ./src/runtime/scheduler/multi_thread_alt/queue.rs:149:13
   4: tokio::runtime::scheduler::multi_thread_alt::worker::Worker::next_remote_task_batch_synced
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:799:9
   5: tokio::runtime::scheduler::multi_thread_alt::worker::Worker::do_park
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:1154:29
   6: tokio::runtime::scheduler::multi_thread_alt::worker::Worker::park
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:1125:30
   7: tokio::runtime::scheduler::multi_thread_alt::worker::Worker::next_task
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:707:46
   8: tokio::runtime::scheduler::multi_thread_alt::worker::Worker::run
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:570:35
   9: tokio::runtime::scheduler::multi_thread_alt::worker::run::{{closure}}::{{closure}}
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:502:23
  10: tokio::runtime::context::scoped::Scoped<T>::set
             at ./src/runtime/context/scoped.rs:40:9
  11: tokio::runtime::context::set_scheduler::{{closure}}
             at ./src/runtime/context.rs:176:26
  12: std::thread::local::LocalKey<T>::try_with
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/thread/local.rs:270:16
  13: std::thread::local::LocalKey<T>::with
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/thread/local.rs:246:9
  14: tokio::runtime::context::set_scheduler
             at ./src/runtime/context.rs:176:9
  15: tokio::runtime::scheduler::multi_thread_alt::worker::run::{{closure}}
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:498:9
  16: tokio::runtime::context::runtime::enter_runtime
             at ./src/runtime/context/runtime.rs:65:16
  17: tokio::runtime::scheduler::multi_thread_alt::worker::run
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:487:5
  18: tokio::runtime::scheduler::multi_thread_alt::worker::create::{{closure}}
             at ./src/runtime/scheduler/multi_thread_alt/worker.rs:334:49
  19: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
             at ./src/runtime/blocking/task.rs:42:21
  20: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll
             at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tracing-0.1.37/src/instrument.rs:272:9
  21: tokio::runtime::task::core::Core<T,S>::poll::{{closure}}
             at ./src/runtime/task/core.rs:334:17
  22: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut
             at ./src/loom/std/unsafe_cell.rs:16:9
  23: tokio::runtime::task::core::Core<T,S>::poll
             at ./src/runtime/task/core.rs:323:13
  24: tokio::runtime::task::harness::poll_future::{{closure}}
             at ./src/runtime/task/harness.rs:485:19
  25: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/panic/unwind_safe.rs:271:9
  26: std::panicking::try::do_call
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs:500:40
  27: __rust_try
  28: std::panicking::try
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs:464:19
  29: std::panic::catch_unwind
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panic.rs:142:14
  30: tokio::runtime::task::harness::poll_future
             at ./src/runtime/task/harness.rs:473:18
  31: tokio::runtime::task::harness::Harness<T,S>::poll_inner
             at ./src/runtime/task/harness.rs:208:27
  32: tokio::runtime::task::harness::Harness<T,S>::poll
             at ./src/runtime/task/harness.rs:153:15
  33: tokio::runtime::task::raw::poll
             at ./src/runtime/task/raw.rs:276:5
  34: tokio::runtime::task::raw::RawTask::poll
             at ./src/runtime/task/raw.rs:200:18
  35: tokio::runtime::task::UnownedTask<S>::run
             at ./src/runtime/task/mod.rs:437:9
  36: tokio::runtime::blocking::pool::Task::run
             at ./src/runtime/blocking/pool.rs:159:9
  37: tokio::runtime::blocking::pool::Inner::run
             at ./src/runtime/blocking/pool.rs:513:17
  38: tokio::runtime::blocking::pool::Spawner::spawn_thread::{{closure}}
             at ./src/runtime/blocking/pool.rs:471:13
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
worker thread panicking; aborting process

I don't think this is caused by this PR.

@Darksonn
Copy link
Contributor

If we are raising the MSRV, then it should happen in its own separate PR.

@NobodyXu

This comment was marked as outdated.

@NobodyXu
Copy link
Contributor Author

If we are raising the MSRV, then it should happen in its own separate PR.

I've opened #5887 to bump tokio MSRV to 1.63

@NobodyXu NobodyXu changed the title Make {Mutex, Notify, OnceCell, RwLock, Semaphore}::const_new available on rust-version >= 1.63 Make {Mutex, Notify, OnceCell, RwLock, Semaphore}::const_new always available on rust-version >= 1.63 Jul 27, 2023
@NobodyXu NobodyXu changed the title Make {Mutex, Notify, OnceCell, RwLock, Semaphore}::const_new always available on rust-version >= 1.63 Make {Mutex, Notify, OnceCell, RwLock, Semaphore}::const_new always available Jul 27, 2023
@Darksonn
Copy link
Contributor

There are some tests using this that currently only run when the parking_lot feature is set. Please make them run always.

@NobodyXu NobodyXu force-pushed the feat/const-new branch 6 times, most recently from 5d805a1 to 66542d0 Compare July 27, 2023 13:58
@NobodyXu
Copy link
Contributor Author

There are some tests using this that currently only run when the parking_lot feature is set. Please make them run always.

Thanks, I've enabled these tests by default and also remove use of cfg tokio_no_const_mutex_new.

@NobodyXu
Copy link
Contributor Author

We can raise the MSRV to 1.63 and remove the trace call from Mutex::new (tracing is unstable).

I think it's better to do this in a separate PR and it will require quite some duplication since rust doesn't permit cfg_attr(not(all(loom, test)), const).

… available

Since MSRV is bumped to 1.63, `Mutex::new` is now usable in const context.

Also use `assert!` in const function to ensure correctness instead of
silently truncating the value and remove cfg `tokio_no_const_mutex_new`.

Signed-off-by: Jiahao XU <[email protected]>
@NobodyXu
Copy link
Contributor Author

Test injection_queue_depth failed in windows CI, I don't think this is caused by this PR and I will open an issue for this.

@Darksonn
Copy link
Contributor

Thanks. I've restarted the failed CI jobs.

@NobodyXu
Copy link
Contributor Author

I've removed the tokio_no_const_mutex_new cfg from CI but it seems that the repository is set to depends on cross-test-with-parking_lot (armv5te-unknown-linux-gnueabi, --cfg tokio_no_const_mutex_new) and cross-test-without-parking_lot (armv5te-unknown-linux-gnueabi, --cfg tokio_no_const_mutex_new).

@Darksonn
Copy link
Contributor

I've removed those checks from the list of required checks.

@NobodyXu
Copy link
Contributor Author

I've removed those checks from the list of required checks.

Thanks!

Now CI is green.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@Darksonn Darksonn merged commit efe3ab6 into tokio-rs:master Jul 28, 2023
73 checks passed
@NobodyXu NobodyXu deleted the feat/const-new branch July 28, 2023 11:49
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 M-sync Module: tokio/sync R-loom Run loom tests on this PR R-loom-sync Run loom sync tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch back to std::sync::Mutex?
4 participants