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

Handle RangeNotSatisfiable for compact index #6675

Merged
merged 2 commits into from Aug 27, 2018
Merged

Conversation

MaxLap
Copy link
Contributor

@MaxLap MaxLap commented Aug 24, 2018

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

On ruby 2.1, bundler would try to install the latest version of psych, which is not compatible with it, instead of using an older version.

This is the error message i would get:
Gem::RuntimeRequirementNotMetError: psych requires Ruby version >= 2.2.2. The current ruby version is 2.1.0.

What was your diagnosis of the problem?

Looking online, i managed to discover that there are 2 API in use, and if bundler can't use the compact index, it will not be able to detect ruby version requirements. (It might be a good idea to have a message that explain that more clearly somewhere, I found that in a random thread somewhere)

Using bundle install --verbose, I found out that a RangeNotSatisfiable was being returned by the info/psych call and it stopped the usage of the compact index. I figured out that this was because the local cache file was too long after reading te code.

I also knew that psych did a mistake in one of their release and had to yank versions in the past.

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

When receiving the RangeNotSatisfiable, instead of generating an exception which bubbles up and forces bundler to switch to the dependency API, I return the response, and test for it in th caller. When the response is that one, I trigger a retry, which reloads the whole while by not requesting a specific range.

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

It seems like the only logical fix to do in the current situation.

If the API didn't use the HTTP header stuff, it would have been more efficient to change rubygems to return the full file or just the new etag instead of a RangeNotSatisfiable. Doing so would have solved the issue for all previous versions of Bundler. But the use of caching servers and so on means that it's better to just respect HTTP and do a second query.

@ghost
Copy link

ghost commented Aug 24, 2018

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

@greysteil
Copy link
Contributor

@MaxLap just to double-check, which version of Bundler are you seeing this on? I only ask because you mention Ruby 2.1, and it's possible the Bundler version being used there is old. I wrote a fix for this that was included in 1.15.2.

@MaxLap
Copy link
Contributor Author

MaxLap commented Aug 24, 2018

I tried it with the most recent one (1.16.4), and it also happens on Travis CI (which uses 1.16.4), here is the job link: https://travis-ci.org/deep-cover/deep-cover/jobs/418477777

Are you talking about #5826? I saw the code and wondered if I should remove it since this PR is a superset fixing-wise. But it could also be considered a performance thing to avoid double query to the server in those cases.

Just to clarify, this is the info/psych that my colleague had. bad_psych_info.txt It's about 7135 bytes long. Where as the current correct one is 6598 bytes long. So doing minus X won't do the trick here.

The errors of the build don't seem related to my changes.

@greysteil
Copy link
Contributor

👍 - I'll take a proper look at this tonight.

Copy link
Member

@segiddins segiddins left a comment

Choose a reason for hiding this comment

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

Thanks for this! Looks like a good safeguard -- we don't expect to get 416s, but if we do, then doing a full download seems like a safe fallback

Bundler.ui.debug("HTTP #{response.code} #{response.message} #{uri}")

case response
when Net::HTTPSuccess, Net::HTTPNotModified
when Net::HTTPSuccess, Net::HTTPNotModified, Net::HTTPRequestedRangeNotSatisfiable
Copy link
Member

Choose a reason for hiding this comment

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

this looks a little suspect to me -- why are we treating 416 responses as successful?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

@MaxLap - I can see what you're doing here, but I think it would be clearer to add retry logic to this method (i.e., set headers["Accept-Encoding"] = "gzip" and then call fetch(..) again in a new when Net::HTTPRequestedRangeNotSatisfiable case.

With that done, lib/bundler/compact_index_client/updater.rb should remain unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if you specify a range and that range is wrong, being told by the server that it was wrong is the normal expected answer in a way.

But I do also agree with what you are saying, and that was my initial fix idea. I just found it clunky to have to alter the headers in a method & class that just receives them and pass them through.

I'll do the change tonight.

@greysteil
Copy link
Contributor

👍 from me, after moving the logic.

FYI, this doesn't replace #5826 - that logic is still necessary to avoid 416s in the happy case (saying "give me the last zero bytes" isn't an acceptable request, which is why we ask for the last one byte). What this does is allow us to use the CompactIndex in the unhappy case. That's a great improvement, though - thanks for the PR!

Fixes issues when gems get yanked, which leads to the cached infos being longer than what is on the server.
Being longer, the compact index api just return a 416, and bundler would fallback to the dependency API.
Retrying to the compact index with no range would fix the issue. This is what this fix does.
@MaxLap
Copy link
Contributor Author

MaxLap commented Aug 27, 2018

The requested change is done. I rebased on master in case that was what made travis fail.

@greysteil
Copy link
Contributor

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 8ddc9ac has been approved by greysteil

@bundlerbot
Copy link
Collaborator

Testing commit 8ddc9ac with merge 4e215b7...

bundlerbot added a commit that referenced this pull request Aug 27, 2018
Handle RangeNotSatisfiable for compact index

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

On ruby 2.1, bundler would try to install the latest version of psych, which is not compatible with it, instead of using an older version.

This is the error message i would get:
`Gem::RuntimeRequirementNotMetError: psych requires Ruby version >= 2.2.2. The current ruby version is 2.1.0.`

### What was your diagnosis of the problem?

Looking online, i managed to discover that there are 2 API in use, and if bundler can't use the compact index, it will not be able to detect ruby version requirements. (It might be a good idea to have a message that explain that more clearly somewhere, I found that in a random thread somewhere)

Using `bundle install --verbose`, I found out that a RangeNotSatisfiable was being returned by the `info/psych` call and it stopped the usage of the compact index. I figured out that this was because the local cache file was too long after reading te code.

I also knew that psych did a mistake in one of their release and had to yank versions in the past.

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

When receiving the `RangeNotSatisfiable`, instead of generating an exception which bubbles up and forces bundler to switch to the dependency API, I return the response, and test for it in th caller. When the response is that one, I trigger a retry, which reloads the whole while by not requesting a specific range.

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

It seems like the only logical fix to do in the current situation.

If the API didn't use the HTTP header stuff, it would have been more efficient to change rubygems to return the full file or just the new etag instead of a RangeNotSatisfiable. Doing so would have solved the issue for all previous versions of Bundler. But the use of caching servers and so on means that it's better to just respect HTTP and do a second query.
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: greysteil
Pushing 4e215b7 to master...

@bundlerbot bundlerbot merged commit 8ddc9ac into rubygems:master Aug 27, 2018
@greysteil
Copy link
Contributor

Thanks for digging into this @MaxLap, and for the PR!

@colby-swandale colby-swandale added this to the 1.16.5 milestone Sep 10, 2018
colby-swandale pushed a commit that referenced this pull request Sep 14, 2018
Handle RangeNotSatisfiable for compact index

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

On ruby 2.1, bundler would try to install the latest version of psych, which is not compatible with it, instead of using an older version.

This is the error message i would get:
`Gem::RuntimeRequirementNotMetError: psych requires Ruby version >= 2.2.2. The current ruby version is 2.1.0.`

### What was your diagnosis of the problem?

Looking online, i managed to discover that there are 2 API in use, and if bundler can't use the compact index, it will not be able to detect ruby version requirements. (It might be a good idea to have a message that explain that more clearly somewhere, I found that in a random thread somewhere)

Using `bundle install --verbose`, I found out that a RangeNotSatisfiable was being returned by the `info/psych` call and it stopped the usage of the compact index. I figured out that this was because the local cache file was too long after reading te code.

I also knew that psych did a mistake in one of their release and had to yank versions in the past.

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

When receiving the `RangeNotSatisfiable`, instead of generating an exception which bubbles up and forces bundler to switch to the dependency API, I return the response, and test for it in th caller. When the response is that one, I trigger a retry, which reloads the whole while by not requesting a specific range.

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

It seems like the only logical fix to do in the current situation.

If the API didn't use the HTTP header stuff, it would have been more efficient to change rubygems to return the full file or just the new etag instead of a RangeNotSatisfiable. Doing so would have solved the issue for all previous versions of Bundler. But the use of caching servers and so on means that it's better to just respect HTTP and do a second query.

(cherry picked from commit 4e215b7)
colby-swandale added a commit that referenced this pull request Sep 18, 2018
* 1-16-stable:
  Version 1.16.5 with changelog
  scope TruffleRuby platform specs to be RubyGems >= 2.1.0
  Auto merge of #6689 - bundler:colby/fix-bundler-load-error, r=colby-swandale
  Auto merge of #6695 - bundler:segiddins/6684-gvp-prefer-non-pres, r=colby-swandale
  Auto merge of #6693 - eregon:truffleruby, r=colby-swandale
  Auto merge of #6692 - eregon:simplify-autoload-require-deprecate, r=segiddins
  Auto merge of #6688 - voxik:check-search, r=colby-swandale
  Auto merge of #6682 - bundler:bundle_env_formatting, r=colby-swandale
  Auto merge of #6675 - MaxLap:master, r=greysteil
  Auto merge of #6669 - ChrisBr:fix_dep_proxy, r=segiddins
  Auto merge of #6664 - greysteil:avoid-printing-git-error, r=colby-swandale
@MaxLap
Copy link
Contributor Author

MaxLap commented Sep 20, 2018

@greysteil I just thought of this but, for the happy case (the files already match), that should be handled by the If-None-Match header and not require #5826.

Doing some testing I see If-None-Match appears to be ignored when Range is given, but reading the HTTP specs, I see that it should not be ignored (last paragraph or Range)... So it seems like this is a bug with Fastly or Rubygems? Any idea of who I should contact about this?

Here are some curl queries to show the problem, might have to update the If-None-Match with the ETag if it changes:

curl -i -H 'If-None-Match: "163daf9bf9777a94c9af1ea7214c8ea8"' https://index.rubygems.org/info/psych
#=> NotModified

curl -i -H 'If-None-Match: "163daf9bf9777a94c9af1ea7214c8ea8"' -H 'Range: bytes=6200-' https://index.rubygems.org/info/psych
#=> PartialContent (Should still be NotModified)

curl -i -H 'If-None-Match: "163daf9bf9777a94c9af1ea7214c8ea8"' -H 'Range: bytes=10000-' https://index.rubygems.org/info/psych
#=> RangeNotSatisfiable (Should still be NotModified)

@indirect
Copy link
Member

@MaxLap I think it’s the Fastly servers returning those responses based on the full file in the cache, so maybe you can use fastly-debug.com to open a ticket with them? I’m happy to join that ticket or file a dupe if that will help.

@MaxLap
Copy link
Contributor Author

MaxLap commented Sep 21, 2018

Alright, thnx. I did a ticket there, we'll see the feedback.

@MaxLap
Copy link
Contributor Author

MaxLap commented Sep 21, 2018

Got an answer, Fastly confirmed the bug. It might take some time to fix it as it's somewhere in the way their servers communicate. I'll keep you posted.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants