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
Avoid crashing when installing from a corrupted lockfile #6355
Avoid crashing when installing from a corrupted lockfile #6355
Conversation
f8e5007
to
5dd28a9
Compare
b40b853
to
6de2b8a
Compare
Thank you, looking good. Normally I try to automatically re-resolve and fix the lockfile if we detect it's corrupted. But it's tricky sometimes, so changing a bad error message with a good error message is great! |
6de2b8a
to
04170b6
Compare
I did a bad thing (script that edits the Gemfile.lock directly) and ended up with a Gemfile.lock that was completely missing some indirect dependencies. While this is my fault and an error is reasonable, I noticed that the error got progressively less friendly in recent versions of bundler. Something similar came up in rubygems#6210, and this commit would have helped with that case as well (although we've already handled this a different way with rubygems#6219). Details: --- Back on Bundler 2.2.23, a corrupt lockfile like this would cause a helpful error: ``` Unable to find a spec satisfying minitest (>= 5.1) in the set. Perhaps the lockfile is corrupted? ``` Bundler 2.3.26 gave a helpful warning: ``` Warning: Your lockfile was created by an old Bundler that left some things out. Because of the missing DEPENDENCIES, we can only install gems one at a time, instead of installing 16 at a time. You can fix this by adding the missing gems to your Gemfile, running bundle install, and then removing the gems from your Gemfile. The missing gems are: * minitest depended upon by activesupport ``` But then continued on and crashed while trying to report the unmet dependency: ``` --- ERROR REPORT TEMPLATE ------------------------------------------------------- NoMethodError: undefined method `full_name' for nil:NilClass lib/bundler/installer/parallel_installer.rb:127:in `block (2 levels) in check_for_unmet_dependencies' ... ``` Bundler 2.4.0 and up crash as above when jobs=1, but crash even harder when run in parallel: ``` --- ERROR REPORT TEMPLATE ------------------------------------------------------- fatal: No live threads left. Deadlock? 3 threads, 3 sleeps current:0x00007fa6b6704660 main thread:0x00007fa6b6704660 * #<Thread:0x000000010833b130 sleep_forever> rb_thread_t:0x00007fa6b6704660 native:0x0000000108985600 int:0 * #<Thread:0x0000000108dea630@Parallel Installer Worker #0 tmp/1/gems/system/gems/bundler-2.5.0.dev/lib/bundler/worker.rb:90 sleep_forever> rb_thread_t:0x00007fa6b67f67c0 native:0x0000700009a62000 int:0 * #<Thread:0x0000000108dea4a0@Parallel Installer Worker rubygems#1 tmp/1/gems/system/gems/bundler-2.5.0.dev/lib/bundler/worker.rb:90 sleep_forever> rb_thread_t:0x00007fa6b67f63c0 native:0x0000700009c65000 int:0 <internal:thread_sync>:18:in `pop' tmp/1/gems/system/gems/bundler-2.5.0.dev/lib/bundler/worker.rb:42:in `deq' ... ``` Changes --- This commit fixes the confusing thread deadlock crash by detecting if dependencies are missing such that we'll never be able to enqueue. When that happens we treat it as a failure so the install can finish. That gets us back to the `NoMethodError`, which this commit fixes by using a different warning in the case where no spec is found.
04170b6
to
d73001a
Compare
Played around with that a bit and it does seems possible. I can do that in a couple weeks when I get back from a vacation. |
That'd be awesome! For now enjoy your vacation ❤️ |
I'm merging this for now regardless since this seems like a strict improvement! |
Avoid crashing when installing from a corrupted lockfile (cherry picked from commit e8c2ed0)
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. Since in this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. Since we've already got some incomplete-spec-auto-healing here, I've treated this as another brand of incomplete spec.
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. Since in this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. Since we've already got some incomplete-spec-auto-healing here, I've treated this as another brand of incomplete spec.
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. Since in this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. We've already got a way to handle this, via `SpecSet#incomplete_specs`, but it wasn't quite working for this case because we'd get to `@incomplete_specs += lookup[name]` and `lookup[name]` would be empty for the dependency. With this commit we catch it a bit earlier, marking the spec containing the missing dependency as incomplete.
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. In this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. We've already got a way to handle this, via `SpecSet#incomplete_specs`, but it wasn't quite working for this case because we'd get to `@incomplete_specs += lookup[name]` and `lookup[name]` would be empty for the dependency. With this commit we catch it a bit earlier, marking the spec containing the missing dependency as incomplete.
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. In this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. We've already got a way to handle this, via `SpecSet#incomplete_specs`, but it wasn't quite working for this case because we'd get to `@incomplete_specs += lookup[name]` and `lookup[name]` would be empty for the dependency. With this commit we catch it a bit earlier, marking the spec containing the missing dependency as incomplete.
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. In this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. We've already got a way to handle this, via `SpecSet#incomplete_specs`, but it wasn't quite working for this case because we'd get to `@incomplete_specs += lookup[name]` and `lookup[name]` would be empty for the dependency. With this commit we catch it a bit earlier, marking the spec containing the missing dependency as incomplete.
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. In this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. We've already got a way to handle this, via `SpecSet#incomplete_specs`, but it wasn't quite working for this case because we'd get to `@incomplete_specs += lookup[name]` and `lookup[name]` would be empty for the dependency. With this commit we catch it a bit earlier, marking the parent spec containing the missing dependency as incomplete.
Following up on rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. In this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. We've already got a way to handle this, via `SpecSet#incomplete_specs`, but it wasn't quite working for this case because we'd get to `@incomplete_specs += lookup[name]` and `lookup[name]` would be empty for the dependency. With this commit we catch it a bit earlier, marking the parent spec containing the missing dependency as incomplete.
Following up on rubygems/rubygems#6355, which turned a crash into a nicer error message, this commit auto-heals the corrupt lockfile instead. In this particular case (a corrupt Gemfile.lock with missing dependencies) the LazySpecification will not have accurate dependency information, we have to materialize the SpecSet to determine there are missing dependencies. We've already got a way to handle this, via `SpecSet#incomplete_specs`, but it wasn't quite working for this case because we'd get to `@incomplete_specs += lookup[name]` and `lookup[name]` would be empty for the dependency. With this commit we catch it a bit earlier, marking the parent spec containing the missing dependency as incomplete. rubygems/rubygems@486ecb8f20
What was the end-user or developer problem that led to this PR?
I did a bad thing (script that edits the Gemfile.lock directly) and ended up with a Gemfile.lock that was completely missing some indirect dependencies. While this is my fault and an error is reasonable, I noticed that the error got progressively less friendly in recent versions of bundler.
Something similar came up in #6210, and this commit would have helped with that case as well (although we've already handled that a different way with #6219).
Previous behavior
Back on Bundler 2.2.23, a corrupt lockfile like this would cause a helpful error:
Bundler 2.2.24 to 2.3.26 gave a helpful (if inaccurate, since an old Bundler was not necessarily at fault) warning:
But then continued on and crashed while trying to report the unmet dependency:
Bundler 2.4.0 and up crash as above when jobs=1, but crash even harder when run in parallel:
Note that these crashes only happen when installing local. When installing from a remote we end up getting an
APIResponseMismatchError
viarubygems/bundler/lib/bundler/remote_specification.rb
Line 55 in 2f48d60
What is your fix for the problem, implemented in this PR?
This commit fixes the confusing thread deadlock crash by detecting if dependencies are missing such that we'll never be able to enqueue. When that happens we treat it as a failure so the install can finish.
That gets us back to the
NoMethodError
, which this commit fixes by using a different warning in the case where no spec is found.Make sure the following tasks are checked