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

Panic with "already borrowed: BorrowMutError" since nightly-2024-03-26 #13646

Closed
taiki-e opened this issue Mar 26, 2024 · 5 comments · Fixed by #13647
Closed

Panic with "already borrowed: BorrowMutError" since nightly-2024-03-26 #13646

taiki-e opened this issue Mar 26, 2024 · 5 comments · Fixed by #13647
Labels
C-bug Category: bug Command-generate-lockfile P-high Priority: High regression-from-stable-to-nightly Regression in nightly that previously worked in stable. S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@taiki-e
Copy link
Member

taiki-e commented Mar 26, 2024

Problem

Since nightly-2024-03-26, -Z minimal-versions and -Z direct-minimal-versions panic on some my projects when lockfile is already exists.

EDIT: this seems to be reproducible without -Z minimal-versions/-Z direct-minimal-versions: #13646 (comment)

Steps

$ git clone https://github.com/taiki-e/easytime
$ cd easytime
$ cargo generate-lockfile
$ cargo update -Z minimal-versions
    Updating crates.io index
 Downgrading aho-corasick v1.1.3 -> v0.6.0 (latest: v1.1.3)
 Downgrading anyhow v1.0.81 -> v1.0.0 (latest: v1.0.81)
thread 'main' panicked at src/cargo/util/context/mod.rs:412:20:
already borrowed: BorrowMutError
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::cell::panic_already_borrowed
   3: <cargo::util::progress::State>::tick
   4: <cargo::sources::registry::http_remote::HttpRegistry as cargo::sources::registry::RegistryData>::block_until_ready
   5: <cargo::sources::registry::RegistrySource as cargo::sources::source::Source>::block_until_ready
   6: <cargo::sources::replaced::ReplacedSource as cargo::sources::source::Source>::block_until_ready
   7: <cargo::core::registry::PackageRegistry as cargo::core::registry::Registry>::block_until_ready
   8: cargo::ops::cargo_generate_lockfile::print_lockfile_updates
   9: cargo::ops::cargo_generate_lockfile::update_lockfile
  10: cargo::commands::update::exec
  11: <cargo::cli::Exec>::exec
  12: cargo::cli::main
  13: cargo::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

It can be work around by removing Cargo.lock before calling -Z minimal-versions.

It can also be reproduced with https://github.com/taiki-e/pin-project-lite.

It can also be reproduced with -Z direct-minimal-versions.

$ cargo minimal-versions build --workspace --no-private --detach-path-deps=skip-exact --all-features --direct
info: running `cargo update -Z direct-minimal-versions`
thread 'main' panicked at src/cargo/util/context/mod.rs:412:20:
already borrowed: BorrowMutError

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.79.0-nightly (a510712d0 2024-03-25)
release: 1.79.0-nightly
commit-hash: a510712d05c6c98f987af24dd73cdfafee8922e6
commit-date: 2024-03-25
host: aarch64-apple-darwin
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.4.0 (sys:0.4.72+curl-8.6.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 14.4.0 [64-bit]
@dtolnay
Copy link
Member

dtolnay commented Mar 26, 2024

I also observed this in https://github.com/dtolnay/rustversion/actions/runs/8429673336/job/23084341781, in which I use neither -Zminimal-versions nor -Zdirect-minimal-versions. Seems like a broader issue. Nightly-2024-03-26 is the first version affected in my case as well.

@taiki-e taiki-e changed the title -Z minimal-versions and -Z direct-minimal-versions panic with "already borrowed: BorrowMutError" since nightly-2024-03-26 Panic with "already borrowed: BorrowMutError" since nightly-2024-03-26 Mar 26, 2024
@weihanglo weihanglo added the regression-from-stable-to-nightly Regression in nightly that previously worked in stable. label Mar 26, 2024
@dtolnay
Copy link
Member

dtolnay commented Mar 26, 2024

Bisects to #13561 and specifically 4ab2797, using cargo test in the rustversion repo. @epage

@taiki-e
Copy link
Member Author

taiki-e commented Mar 26, 2024

From a quick look at the panic location (GlobalContext::shell), backtrace, and #13561 diff, it appears to be related to the fact that BorrowMut (returned by GlobalContext::shell), which previously persisted only during printing, now persists for the entire print_lockfile_updates.

@weihanglo weihanglo added P-high Priority: High Command-generate-lockfile S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-triage Status: This issue is waiting on initial triage. labels Mar 26, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 26, 2024
Update cargo

1 commits in a510712d05c6c98f987af24dd73cdfafee8922e6..499a61ce7a0fc6a72040084862a68b2603e770e8
2024-03-25 03:45:54 +0000 to 2024-03-26 04:17:04 +0000
- fix: do not borrow shell across registry query (rust-lang/cargo#13647)

r? ghost

<hr>

This update includes a fix for a panic within cargo that was noted in [cargo#13646](rust-lang/cargo#13646)
@epage epage added P-high Priority: High regression-from-stable-to-nightly Regression in nightly that previously worked in stable. and removed P-high Priority: High regression-from-stable-to-nightly Regression in nightly that previously worked in stable. labels Mar 26, 2024
@epage
Copy link
Contributor

epage commented Mar 26, 2024

So it appears the reason manual testing and the test suite didn't hit this is there wasn't contention over the package lock. In fact, we only have one test that even attempts to capture the Blocking status.

We could try to make a specific test case to prevent regressions in this one case but then we'd likely run into this in other places. As an alternative, we could do a useless borrow of shell() further up the stack where it'd be unconditional so tests would be more likely to catch this.

@weihanglo
Copy link
Member

we could do a useless borrow of shell() further up the stack where it'd be unconditional so tests would be more likely to catch this.

I wonder How it looks like and how it applies to other RefCell usages in the codebase.

epage added a commit to epage/cargo that referenced this issue Mar 26, 2024
This intentionally borrows from `RefCell`s before conditional code
to try to prevent regressions like rust-lang#13646 without exhaustively
covering every case that could hit these `BorrowMutError`s with
complicated tests.

I tested this by ensuring the registry code path panicked before
rebasing on top of rust-lang#13647.
Once I rebased, the panic went away.
weihanglo pushed a commit to weihanglo/cargo that referenced this issue Mar 26, 2024
This intentionally borrows from `RefCell`s before conditional code
to try to prevent regressions like rust-lang#13646 without exhaustively
covering every case that could hit these `BorrowMutError`s with
complicated tests.

I tested this by ensuring the registry code path panicked before
rebasing on top of rust-lang#13647.
Once I rebased, the panic went away.

Co-authored-by: Ed Page <eopage@gmail.com>
bors added a commit that referenced this issue Mar 26, 2024
test: Add asserts to catch BorrowMutError's

### What does this PR try to resolve?

This intentionally borrows from `RefCell`s before conditional code to try to prevent regressions like #13646 without exhaustively covering every case that could hit these `BorrowMutError`s with complicated tests.

### How should we test and review this PR?

I tested this by ensuring the registry code path panicked before rebasing on top of #13647.
Once I rebased, the panic went away.

### Additional information

Split off from #13649 to try to avoid appveyor
charmitro pushed a commit to charmitro/cargo that referenced this issue Sep 13, 2024
This intentionally borrows from `RefCell`s before conditional code
to try to prevent regressions like rust-lang#13646 without exhaustively
covering every case that could hit these `BorrowMutError`s with
complicated tests.

I tested this by ensuring the registry code path panicked before
rebasing on top of rust-lang#13647.
Once I rebased, the panic went away.

Co-authored-by: Ed Page <eopage@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug Command-generate-lockfile P-high Priority: High regression-from-stable-to-nightly Regression in nightly that previously worked in stable. S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants