Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Dependency resolution error message generator consumes ridiculous amounts of memory for large conflict trees #6114

Closed
halfbyte opened this issue Oct 20, 2017 · 5 comments

Comments

@halfbyte
Copy link
Contributor

halfbyte commented Oct 20, 2017

Debugging a failing dependency update for a Depfu client, I found out that if the conflict tree gets to a certain size, the code to display the reduced conflict set consumes way too much memory. I've never let it finish but usually stopped the code when reaching the 4GB mark.

I've identified the following change as the culprit:

5ebc8c1#diff-a068dff3f4ba60cbf2c11bc4e65eed02R27

The code moved a bit since then and is currently at https://github.com/bundler/bundler/blob/f093e405a8e10bafa0b5d4621069a0b7ad86c37f/lib/bundler/resolver.rb#L311

The issue here is the use of Array#combination. The specific conflict trees array had a length of 26. Here's a simple test case to consume a ton of memory on your machine to exemplify the problem:

([1] * 26).combination(12)

(and please note that the bundler code from above would do that for every value from 1 to 26 as input for Array#combination.)

How to fix this?

One thing I noticed is that the unreduced "trees" variable contains a lot of 1:1 dupes. I've removed them with the following (not very elegant) code, inserted before line 310:

trees.uniq! { |tree| [tree.first, tree.last] }

As my knowledge of the resolving process is limited, I am not sure if this will somehow ruin the intended output. It does bring down my 26 trees to 12 after which the reduction still takes a good 10 seconds or so on my machine but consumes way less memory. Not optimal, but better.

As I am afraid that the 26 I encountered are not the theoretical maximum of conflict trees, I think it would be a good idea to add a bailout clause, so that if the deduped trees size is > 10 or so, the reduction is skipped, which would change the full lambda to:

trees.uniq! {|tree| [tree.first, tree.last] }
if trees.size <= 10
  maximal = 1.upto(trees.size).map do |size|
    trees.map(&:last).flatten(1).combination(size).to_a
  end.flatten(1).select do |deps|
    Bundler::VersionRanges.empty?(*Bundler::VersionRanges.for_many(deps.map(&:requirement)))
  end.min_by(&:size)
  trees.reject! {|t| !maximal.include?(t.last) } if maximal

  trees = trees.sort_by {|t| t.flatten.map(&:to_s) }
  trees.uniq! {|t| t.flatten.map {|dep| [dep.name, dep.requirement] } }

  trees = trees.sort_by {|t| t.reverse.map(&:name) }
end
trees

I'm happy to provide a PR if this sounds plausible to y'all. Not sure I am able to provide a non synthetic example (need to check that with our client), though. I'm pretty sure I'll need some guidance in terms of how to properly test this.

Of course this is a special case as the Gemfile is a particularly large one, but the one gem that makes the conflict trees so large is of course ActiveSupport and yeah, maybe it's not such a special case after all. I think it is fair to assume that any significantly large rails application with lots of "rails plugins" might run into similar issues when trying to forcefully update a dependency depending on a higher version of ActiveSupport.

@halfbyte halfbyte changed the title Dependency resolution error message consumes ridiculous amounts of memory on large conflict trees Dependency resolution error message generator consumes ridiculous amounts of memory for large conflict trees Oct 20, 2017
@segiddins
Copy link
Member

Thanks for the issue! I think we'd need an example to be able to "fix" it, but in the meantime bailing out after a certain point seems like a good idea

@halfbyte
Copy link
Contributor Author

@segiddins Ok, let me try to provide a synthetic extraction from my client's Gemfile. Should be doable. But first: Sleep. :)

@halfbyte
Copy link
Contributor Author

@segiddins Okay, here we go: This Gist contains a Gemfile/Gemfile.lock that will cause the error. Steps to repro:

  1. Download gist content into a folder
  2. cd into folder
  3. run bundle update grape-entity in that folder

What you should see is massive memory usage and almost infinite run time. (Actually I have no idea how long this will run, I never let it finish)

The two fixes outlined above seem to work for me. Again, I'm unsure about the uniq, as I don't know if this will suppress valuable info (my guess is no, since different routes through the dependencies will end up with different instances at the end of the graphs).

As I said, I am willing to prepare a PR, if you could assist and advise me on a testing strategy for this.

@segiddins
Copy link
Member

I think at first a PR capping the number of conflict trees and not trying to reduce them after that point would be a good place to start -- we could add a test after merging, and then try to make the algorithm more efficient on larger inputs afterwords.

bundlerbot added a commit that referenced this issue Oct 29, 2017
Update vendored Molinillo to 0.6.4

### What was the end-user problem that led to this PR?

The problem was uncovered in #6114.

### What is your fix for the problem, implemented in this PR?

My fix updates molinillo to 0.6.4.

### Why did you choose this fix out of the possible options?

See https://github.com/CocoaPods/Molinillo/releases/tag/0.6.4 for release notes.
segiddins pushed a commit that referenced this issue Oct 30, 2017
Update vendored Molinillo to 0.6.4

### What was the end-user problem that led to this PR?

The problem was uncovered in #6114.

### What is your fix for the problem, implemented in this PR?

My fix updates molinillo to 0.6.4.

### Why did you choose this fix out of the possible options?

See https://github.com/CocoaPods/Molinillo/releases/tag/0.6.4 for release notes.

(cherry picked from commit b20214d)
halfbyte added a commit to halfbyte/bundler that referenced this issue Nov 2, 2017
This is a simple fix for rubygems#6114, in which sometimes conflict sets get so huge that
the current reducing code (which is based on Array#combination) breaks down.

By simply bailing out when the conflict set is bigger than a certain size,
we'll get a result that is similar to what happened in earlier bundler versions
but skip a ton of CPU cycles and memory allocations.

I've chosen the limit rather unscientifically by playing around with the result
set sizes. 15 seems to be a good compromise but really anything larger than 10
should work (and with work I mean "should not usually not have to be invoked").
halfbyte added a commit to halfbyte/bundler that referenced this issue Nov 2, 2017
This is a simple fix for rubygems#6114, in which sometimes conflict sets get so huge that
the current reducing code (which is based on Array#combination) breaks down.

By simply bailing out when the conflict set is bigger than a certain size,
we'll get a result that is similar to what happened in earlier bundler versions
but skip a ton of CPU cycles and memory allocations.

I've chosen the limit rather unscientifically by playing around with the result
set sizes. 15 seems to be a good compromise but really anything larger than 10
should work (and with work I mean "should not usually not have to be invoked").
bundlerbot added a commit that referenced this issue Nov 4, 2017
…iddins

Bail out of reducing depency trees on huge dependency conflict sets

### What was the end-user problem that led to this PR?

The problem is described in #6114 - An unusually large set of conflicting dependencies leads to bundler get stuck and lead to very unhealthy amounts of memory and CPU time be consumed.

### What was your diagnosis of the problem?

This happens because because a too large array is fed into Array#combination in the "reduce_trees" lambda in `version_conflict_message`.

### What is your fix for the problem, implemented in this PR?

By simply bailing out when the conflict set is bigger than a certain size,
we'll get a result that is similar to what happened in earlier bundler versions
but skip a ton of CPU cycles and memory allocations.

I've chosen the limit rather unscientifically by playing around with the result
set sizes. 15 seems to be a good compromise but really anything larger than 10
should work (and with work I mean "should not usually not have to be invoked").

### Why did you choose this fix out of the possible options?

Reducing the conflict trees has been a rather new addition to bundler, so defaulting back to the old behavior of not reducing the trees seems like an OK option. It may be possible to also optimize the reduction algorithm, but since this is very much an edge case that only happens when using Bundler slightly out of the normal procedures, I think a simple bail out is sufficient.
@indirect
Copy link
Member

Fixed by #6145

segiddins pushed a commit that referenced this issue Dec 12, 2017
…iddins

Bail out of reducing depency trees on huge dependency conflict sets

### What was the end-user problem that led to this PR?

The problem is described in #6114 - An unusually large set of conflicting dependencies leads to bundler get stuck and lead to very unhealthy amounts of memory and CPU time be consumed.

### What was your diagnosis of the problem?

This happens because because a too large array is fed into Array#combination in the "reduce_trees" lambda in `version_conflict_message`.

### What is your fix for the problem, implemented in this PR?

By simply bailing out when the conflict set is bigger than a certain size,
we'll get a result that is similar to what happened in earlier bundler versions
but skip a ton of CPU cycles and memory allocations.

I've chosen the limit rather unscientifically by playing around with the result
set sizes. 15 seems to be a good compromise but really anything larger than 10
should work (and with work I mean "should not usually not have to be invoked").

### Why did you choose this fix out of the possible options?

Reducing the conflict trees has been a rather new addition to bundler, so defaulting back to the old behavior of not reducing the trees seems like an OK option. It may be possible to also optimize the reduction algorithm, but since this is very much an edge case that only happens when using Bundler slightly out of the normal procedures, I think a simple bail out is sufficient.

(cherry picked from commit fc341ed)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Mar 14, 2018
## 1.16.1 (2017-12-12)

Bugfixes:

  - avoid hanging on complex resolver errors ([#6114](rubygems/bundler#6114), @halfbyte)
  - avoid an error when running `bundle update --group` ([#6156](rubygems/bundler#6156), @mattbrictson)
  - ensure the resolver prefers non-pre-release gems when possible ([#6181](rubygems/bundler#6181), @greysteil)
  - include bundler's gemspec in the built gem ([#6165](rubygems/bundler#6165), @dr-itz)
  - ensure locally installed specs are not overriden by those in remote sources during dependency resolution ([#6072](rubygems/bundler#6072), @indirect)
  - ensure custom gemfiles are respected in generated binstubs (@pftg)
  - fail gracefully when loading a bundler-generated binstub when `bin/bundle` was not generated by bundler ([#6149](rubygems/bundler#6149), @hsbt)
  - allow `bundle init` to be run even when a parent directory contains a gemfile ([#6205](rubygems/bundler#6205), @colby-swandale)

## 1.16.0 (2017-10-31)

Bugfixes:

  - avoid new RubyGems warning about unsafe YAML loading (to keep output consistent) (@segiddins)
  - load digest subclasses in a thread-safe manner (@segiddins, @colby-swandale)
  - avoid unusued variable warnings under ruby 2.5 (@amatsuda)
  - fix printing the same message twice in verbose mode ([#6028](rubygems/bundler#6028), @akhramov)
  - allow `SignalException`s to bubble up to the interpreter during `bundle exec` ([#6090](rubygems/bundler#6090), @dekellum)
  - avoid activating stdlib digest under Ruby 2.5 (@segiddins)
  - prioritise explicitly requested gems in dependency resolution sort order (@segiddins)
  - reduce memory usage during dependency resolution ([#6114](rubygems/bundler#6114), @greysteil)
  - ensure that the default bundler gem is not accidentally activated on ruby 2.5 when using local git overrides (@segiddins)

## 1.16.0.pre.3 (2017-10-04)

Features:

  - the output from `bundle env` includes more information, particularly both the compiled & loaded versions of OpenSSL (@indirect)

Bugfixes:

  - fix a bug where installing on FreeBSD would accidentally raise an error (#6013, @olleolleolle)
  - fix a regression in 1.16 where pre-release gems could accidentally be resolved even when the gemfile contained no pre-release requirements (@greysteil)
  - bundler will avoid making unnecessary network requests to fetch dependency data, fixing a regression introduced in 1.16 (@segiddins)
  - the outdated bundler version message is disabled by default until the message has been fine-tuned (#6004, @segiddins)

## 1.16.0.pre.2 (2017-09-06)

Bugfixes:

  - handle when a connection is missing a socket when warning about OpenSSL version (@greysteil)
  - the description for the `rake release` task now reflects `$RUBYGEMS_HOST` (@wadetandy)
  - fix a bug where `bundle update` would regress transitive dependencies (@greysteil)

## 1.16.0.pre.1 (2017-09-04)

Features:

  - allow using non-branch symbolic refs in a git source (#4845, @segiddins)
  - allow absolute paths in the `cache path` setting (#5627, @mal)
  - gems created via `bundle gem` with rspec have `--require spec_helper` in their `.rspec` file (@koic)
  - `bundle env` includes `Gem.ruby` and the `bundle` binstub shebang when they don't match (#5616, @segiddins)
  - allow passing gem names to `bundle pristine` (@segiddins)
  - `bundle version` and `bundle env` include the commit and build date for the bundler gem (#5049, @segiddins)
  - add the `--shebang` option to `bundle binstubs` (#4070, @segiddins, @zorbash)
  - gemfiles are `eval`ed one fewer time when running `bundle install` (#4952, #3096, #4417, @segiddins)
  - the `fileutils` gem is now vendored so different versions of the gem can be activated (@segiddins)
  - speed up no-op installations (#5842, @segiddins)
  - default to keeping the lockfile in the default gem template (@deivid-rodriguez)
  - add a special bundler binstub that ensures the correct version of bundler is activated (#5876, @segiddins)
  - speed up dependency resolution and ensure that all resolvable gemfiles can be installed (@segiddins, @greysteil)
  - add a `bundle list` command that prints the gems in use (#4754, @colby-swandale)
  - allow adding credentials to a gem source during deployment when `allow_deployment_source_credential_changes` is set (@adrian-gomez)
  - making an outdated (and insecure) TLS connection to rubygems.org will print a warning (@segiddins)

Bugfixes:

  - allow configuring a mirror fallback timeout without a trailing slash (#4830, @segiddins)
  - fix handling of mirrors for file: urls that contain upper-case characters (@segiddins)
  - list the correct gem host for `rake release` when `allowed_push_host` has been set (@mdeering)
  - ensure `Bundler.original_env` preserves all env keys that bundler sets (#5700, @segiddins)
  - ensure `bundle pristine` removes files added to a git gem (@segiddins)
  - load plugin files from path gems before gem installation (#5429, @segiddins)
  - ensure gems containing manpages are properly set up (#5730, @segiddins)
  - avoid fetching remote specs when all effected gems are in groups that are not being installed (@segiddins)
  - allow `BUNDLE_GEMFILE` to be a relative path (#5712, @gxespino)
  - show a more helpful error message when a gem fails to install due to a corrupted lockfile (#5846, @segiddins)
  - add a process lock to allow multiple concurrent `bundle install`s (#5851, @stefansedich)
  - ensure that specifications always return an array for `#extensions` (@greysteil)
  - print a helpful error message when using a gem in the Gemfile with an empty name (@colby-swandale)
  - ensure that all gemfiles are included in `bundle env` (@segiddins)
  - use ssl client cert and ca cert settings from gem configuration as fallbacks (@stan3)
  - avoid global namespace pollution when loading gems (#5958, @shyouhei)
  - avoid running a complete re-resolve on `bundle update --bundler` (@segiddins)
  - allow `bundle binstubs --standalone` to work without `path` being set (@colby-swandale)
  - fix support for bundle paths that include jars or wars on jruby (#5975, @torcido)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants