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

Handle RangeNotSatisfiable for compact index #6675

Merged
merged 2 commits into from Aug 27, 2018

Conversation

Projects
None yet
6 participants
@MaxLap
Contributor

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.

@bundlerbot

This comment has been minimized.

bundlerbot bot 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

@MaxLap MaxLap force-pushed the MaxLap:master branch from 017f52d to 5f15b4a Aug 24, 2018

@greysteil

This comment has been minimized.

Member

greysteil commented Aug 24, 2018

@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 MaxLap force-pushed the MaxLap:master branch from 5f15b4a to 935615e Aug 24, 2018

@MaxLap

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

greysteil commented Aug 24, 2018

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

@segiddins

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

This comment has been minimized.

@segiddins

segiddins Aug 27, 2018

Member

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

This comment has been minimized.

@greysteil

greysteil Aug 27, 2018

Member

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.

This comment has been minimized.

@MaxLap

MaxLap Aug 27, 2018

Contributor

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

This comment has been minimized.

Member

greysteil commented Aug 27, 2018

👍 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!

MaxLap added some commits Aug 21, 2018

Let updater retry on HTTPRequestedRangeNotSatisfiable (416)
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 MaxLap force-pushed the MaxLap:master branch from 935615e to 8ddc9ac Aug 27, 2018

@MaxLap

This comment has been minimized.

Contributor

MaxLap commented Aug 27, 2018

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

@greysteil

This comment has been minimized.

Member

greysteil commented Aug 27, 2018

@bundlerbot

This comment has been minimized.

Contributor

bundlerbot commented Aug 27, 2018

📌 Commit 8ddc9ac has been approved by greysteil

@bundlerbot

This comment has been minimized.

Contributor

bundlerbot commented Aug 27, 2018

⌛️ Testing commit 8ddc9ac with merge 4e215b7...

bundlerbot added a commit that referenced this pull request Aug 27, 2018

Auto merge of #6675 - MaxLap:master, r=greysteil
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

This comment has been minimized.

Contributor

bundlerbot commented Aug 27, 2018

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

@bundlerbot bundlerbot merged commit 8ddc9ac into bundler:master Aug 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@greysteil

This comment has been minimized.

Member

greysteil commented Aug 27, 2018

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 added a commit that referenced this pull request Sep 14, 2018

Auto merge of #6675 - MaxLap:master, r=greysteil
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

merge v1.16.5
* 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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

indirect commented Sep 20, 2018

@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

This comment has been minimized.

Contributor

MaxLap commented Sep 21, 2018

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

@MaxLap

This comment has been minimized.

Contributor

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 join this conversation on GitHub. Already have an account? Sign in to comment