yarnpkg · arcanis · Sep 25, 2018 · Sep 21, 2018 · Sep 21, 2018 · Sep 21, 2018
diff --git a/__tests__/util/request-manager.js b/__tests__/util/request-manager.js
@@ -183,27 +183,67 @@ test('RequestManager.execute timeout error with default maxRetryAttempts', async
   }
 });
 
-test('RequestManager.execute Request 403 error', async () => {
-  const config = await Config.create({}, new Reporter());
-  jest.mock('request', factory => options => {
-    options.callback('', {statusCode: 403}, '');
-    return {
-      on: () => {},
-    };
+for (const statusCode of [403, 442]) {
+  test(`RequestManager.execute Request ${statusCode} error`, async () => {
+    // The await await is just to silence Flow - https://github.com/facebook/flow/issues/6064
+    const config = await await Config.create({}, new Reporter());
+    const mockStatusCode = statusCode;
+    jest.mock('request', factory => options => {
+      options.callback('', {statusCode: mockStatusCode}, '');
+      return {
+        on: () => {},
+      };
+    });
+    await config.requestManager.execute({
+      params: {
+        url: `https://localhost:port/?nocache`,
+        headers: {Connection: 'close'},
+      },
+      resolve: body => {},
+      reject: err => {
+        expect(err.message).toBe(
+          `https://localhost:port/?nocache: Request "https://localhost:port/?nocache" returned a 403`,
+        );
+      },
+    });
   });
-  await config.requestManager.execute({
-    params: {
-      url: `https://localhost:port/?nocache`,
-      headers: {Connection: 'close'},
-    },
-    resolve: body => {},
-    reject: err => {
-      expect(err.message).toBe(
-        'https://localhost:port/?nocache: Request "https://localhost:port/?nocache" returned a 403',
-      );
-    },
+}
+
+// Cloudflare will occasionally return an html response with a 500 status code on some calls
+for (const statusCode of [408, 500, 542]) {
+  test(`RequestManager.execute retries on ${statusCode} error`, async () => {
+    jest.resetModules();
+    // The await await is just to silence Flow - https://github.com/facebook/flow/issues/6064
+    const config = await await Config.create({}, new Reporter());
+    const mockStatusCode = statusCode;
+    jest.mock('request', factory => {
+      let retryCount = 2;
+      return options => {
+        if (retryCount-- > 0) {
+          options.callback(
+            '',
+            {statusCode: mockStatusCode},
+            `<!DOCTYPE html><title>Rendering error | registry.yarnpkg.com | Cloudflare</title>...`,
+          );
+        } else {
+          options.callback('', {statusCode: 200}, '');
+        }
+        return {
+          on: () => {},
+        };
+      };
+    });
+    await config.requestManager.execute({
+      params: {
+        url: `https://localhost:port/?nocache`,
+        headers: {Connection: 'close'},
+      },
+      resolve: body => {
+        expect(body).not.toEqual(false);
+      },
+    });
   });
-});
+}
 
 test('RequestManager.request with offlineNoRequests', async () => {
   const config = await Config.create({offline: true}, new Reporter());

diff --git a/src/config.js b/src/config.js
@@ -374,7 +374,7 @@ export default class Config {
     this.linkFileDependencies = Boolean(this.getOption('yarn-link-file-dependencies'));
     this.packBuiltPackages = Boolean(this.getOption('experimental-pack-script-packages-in-mirror'));
 
-    this.autoAddIntegrity = !Boolean(this.getOption('unsafe-disable-integrity-migration'));
+    this.autoAddIntegrity = !this.getOption('unsafe-disable-integrity-migration');
 
     //init & create cacheFolder, tempFolder
     this.cacheFolder = path.join(this._cacheRootFolder, 'v' + String(constants.CACHE_VERSION));

diff --git a/src/fetchers/tarball-fetcher.js b/src/fetchers/tarball-fetcher.js
@@ -1,12 +1,11 @@
 /* @flow */
 
-import http from 'http';
-import {SecurityError, MessageError, ResponseError} from '../errors.js';
+import {SecurityError, MessageError} from '../errors.js';
 import type {FetchedOverride} from '../types.js';
 import * as constants from '../constants.js';
 import BaseFetcher from './base-fetcher.js';
 import * as fsUtil from '../util/fs.js';
-import {removePrefix, sleep} from '../util/misc.js';
+import {removePrefix} from '../util/misc.js';
 
 const crypto = require('crypto');
 const path = require('path');
@@ -227,73 +226,50 @@ export default class TarballFetcher extends BaseFetcher {
   async fetchFromExternal(): Promise<FetchedOverride> {
     const registry = this.config.registries[this.registry];
 
-    let retriesRemaining = 2;
-    do {
-      try {
-        return await registry.request(
-          this.reference,
-          {
-            headers: {
-              'Accept-Encoding': 'gzip',
-            },
-            buffer: true,
-            process: (req, resolve, reject) => {
-              // should we save this to the offline cache?
-              const {reporter} = this.config;
-              const tarballMirrorPath = this.getTarballMirrorPath();
-              const tarballCachePath = this.getTarballCachePath();
-
-              const {validateStream, extractorStream} = this.createExtractor(resolve, reject);
-
-              req.on('response', res => {
-                if (res.statusCode >= 400) {
-                  const statusDescription = http.STATUS_CODES[res.statusCode];
-                  reject(
-                    new ResponseError(
-                      reporter.lang('requestFailed', `${res.statusCode} ${statusDescription}`),
-                      res.statusCode,
-                    ),
-                  );
-                }
-              });
-              req.pipe(validateStream);
-
-              if (tarballMirrorPath) {
-                validateStream.pipe(fs.createWriteStream(tarballMirrorPath)).on('error', reject);
-              }
+    try {
+      return await registry.request(
+        this.reference,
+        {
+          headers: {
+            'Accept-Encoding': 'gzip',
+          },
+          buffer: true,
+          process: (req, resolve, reject) => {
+            // should we save this to the offline cache?
+            const tarballMirrorPath = this.getTarballMirrorPath();
+            const tarballCachePath = this.getTarballCachePath();
 
-              if (tarballCachePath) {
-                validateStream.pipe(fs.createWriteStream(tarballCachePath)).on('error', reject);
-              }
+            const {validateStream, extractorStream} = this.createExtractor(resolve, reject);
 
-              validateStream.pipe(extractorStream).on('error', reject);
-            },
-          },
-          this.packageName,
-        );
-      } catch (err) {
-        if (err instanceof ResponseError && err.responseCode >= 500 && retriesRemaining > 1) {
-          retriesRemaining--;
-          this.reporter.warn(this.reporter.lang('retryOnInternalServerError'));
-          await sleep(3000);
-        } else {
-          const tarballMirrorPath = this.getTarballMirrorPath();
-          const tarballCachePath = this.getTarballCachePath();
+            req.pipe(validateStream);
 
-          if (tarballMirrorPath && (await fsUtil.exists(tarballMirrorPath))) {
-            await fsUtil.unlink(tarballMirrorPath);
-          }
+            if (tarballMirrorPath) {
+              validateStream.pipe(fs.createWriteStream(tarballMirrorPath)).on('error', reject);
+            }
 
-          if (tarballCachePath && (await fsUtil.exists(tarballCachePath))) {
-            await fsUtil.unlink(tarballCachePath);
-          }
+            if (tarballCachePath) {
+              validateStream.pipe(fs.createWriteStream(tarballCachePath)).on('error', reject);
+            }
 
-          throw err;
-        }
+            validateStream.pipe(extractorStream).on('error', reject);
+          },
+        },
+        this.packageName,
+      );
+    } catch (err) {
+      const tarballMirrorPath = this.getTarballMirrorPath();
+      const tarballCachePath = this.getTarballCachePath();
+
+      if (tarballMirrorPath && (await fsUtil.exists(tarballMirrorPath))) {
+        await fsUtil.unlink(tarballMirrorPath);
       }
-    } while (retriesRemaining > 0);
-    // Unreachable code, this is just to make Flow happy
-    throw new Error('Ran out of retries!');
+
+      if (tarballCachePath && (await fsUtil.exists(tarballCachePath))) {
+        await fsUtil.unlink(tarballCachePath);
+      }
+
+      throw err;
+    }
   }
 
   _fetch(): Promise<FetchedOverride> {

diff --git a/src/reporters/lang/en.js b/src/reporters/lang/en.js
@@ -21,6 +21,7 @@ const messages = {
   waitingInstance: 'Waiting for the other yarn instance to finish (pid $0, inside $1)',
   waitingNamedInstance: 'Waiting for the other yarn instance to finish ($0)',
   offlineRetrying: 'There appears to be trouble with your network connection. Retrying...',
+  internalServerErrorRetrying: 'There appears to be trouble with the npm registry (returned $1). Retrying...',
   clearedCache: 'Cleared cache.',
   couldntClearPackageFromCache: "Couldn't clear package $0 from cache",
   clearedPackageFromCache: 'Cleared package $0 from cache',
@@ -348,7 +349,6 @@ const messages = {
   errorExtractingTarball: 'Extracting tar content of $1 failed, the file appears to be corrupt: $0',
   updateInstalling: 'Installing $0...',
   hostedGitResolveError: 'Error connecting to repository. Please, check the url.',
-  retryOnInternalServerError: 'There appears to be trouble with our server. Retrying...',
 
   unknownFetcherFor: 'Unknown fetcher for $0',
 

diff --git a/src/util/request-manager.js b/src/util/request-manager.js
@@ -1,13 +1,14 @@
 /* @flow */
 
 import fs from 'fs';
+import http from 'http';
 import url from 'url';
 import dnscache from 'dnscache';
 import invariant from 'invariant';
 import RequestCaptureHar from 'request-capture-har';
 
 import type {Reporter} from '../reporters/index.js';
-import {MessageError} from '../errors.js';
+import {MessageError, ResponseError} from '../errors.js';
 import BlockingQueue from './blocking-queue.js';
 import * as constants from '../constants.js';
 import * as network from './network.js';
@@ -67,6 +68,7 @@ type RequestParams<T> = {
 };
 
 type RequestOptions = {
+  retryReason: ?string,
   params: RequestParams<Object>,
   resolve: (body: any) => void,
   reject: (err: any) => void,
@@ -311,9 +313,23 @@ export default class RequestManager {
    * isn't already one.
    */
 
-  queueForOffline(opts: RequestOptions) {
+  queueForRetry(opts: RequestOptions) {
+    if (opts.retryReason) {
+      let containsReason = false;
+
+      for (const queuedOpts of this.offlineQueue) {
+        if (queuedOpts.retryReason === opts.retryReason) {
+          containsReason = true;
+          break;
+        }
+      }
+
+      if (!containsReason) {
+        this.reporter.info(opts.retryReason);
+      }
+    }
+
     if (!this.offlineQueue.length) {
-      this.reporter.info(this.reporter.lang('offlineRetrying'));
       this.initOfflineRetry();
     }
 
@@ -357,22 +373,34 @@ export default class RequestManager {
       rejectNext(err);
     };
 
+    const queueForRetry = reason => {
+      const attempts = params.retryAttempts || 0;
+      if (attempts >= this.maxRetryAttempts - 1) {
+        return false;
+      }
+      if (opts.params.method && opts.params.method.toUpperCase() !== 'GET') {
+        return false;
+      }
+      params.retryAttempts = attempts + 1;
+      if (typeof params.cleanup === 'function') {
+        params.cleanup();
+      }
+      opts.retryReason = reason;
+      this.queueForRetry(opts);
+      return true;
+    };
+
     let calledOnError = false;
     const onError = err => {
       if (calledOnError) {
         return;
       }
       calledOnError = true;
 
-      const attempts = params.retryAttempts || 0;
-      if (attempts < this.maxRetryAttempts - 1 && this.isPossibleOfflineError(err)) {
-        params.retryAttempts = attempts + 1;
-        if (typeof params.cleanup === 'function') {
-          params.cleanup();
+      if (this.isPossibleOfflineError(err)) {
+        if (!queueForRetry(this.reporter.lang('offlineRetrying'))) {
+          reject(err);
         }
-        this.queueForOffline(opts);
-      } else {
-        reject(err);
       }
     };
 
@@ -389,18 +417,27 @@ export default class RequestManager {
 
         this.reporter.verbose(this.reporter.lang('verboseRequestFinish', params.url, res.statusCode));
 
+        if (res.statusCode === 408 || res.statusCode >= 500) {
+          const description = `${res.statusCode} ${http.STATUS_CODES[res.statusCode]}`;
+          if (!queueForRetry(this.reporter.lang('internalServerErrorRetrying', description))) {
+            throw new ResponseError(this.reporter.lang('requestFailed', description), res.statusCode);
+          } else {
+            return;
+          }
+        }
+
         if (body && typeof body.error === 'string') {
           reject(new Error(body.error));
           return;
         }
 
-        if (res.statusCode === 403) {
+        if ([400, 401, 404].concat(params.rejectStatusCode || []).indexOf(res.statusCode) !== -1) {
+          // So this is actually a rejection ... the hosted git resolver uses this to know whether http is supported
+          resolve(false);
+        } else if (res.statusCode >= 400) {
           const errMsg = (body && body.message) || reporter.lang('requestError', params.url, res.statusCode);
           reject(new Error(errMsg));
         } else {
-          if ([400, 401, 404].concat(params.rejectStatusCode || []).indexOf(res.statusCode) !== -1) {
-            body = false;
-          }
           resolve(body);
         }
       };