Retry is broken

[TomiTakussaari]

Expected Behavior

By default, this SDK is retrying requests which resulted in a 500 server error and 429 rate limit response.

Actual Behavior

Contentful.js does not seem to retry errors:

import nock from 'nock';
import * as contentful from 'contentful';

test('contentful client retries on network errors', async () => {
  const contentfulConfig = {
    accessToken: 'foobar',
    environment: 'master',
    space: 'my-space',
    retryOnError: true
  };
  const contentClient = contentful.createClient(contentfulConfig);
  nock('https://cdn.contentful.com')
    .get('/spaces/my-space/environments/master/entries?content_type=article')
    .replyWithError({'message': 'something awful happened', code: 'ECONNRESET'})
    .get('/spaces/my-space/environments/master/entries?content_type=article')
    .reply(200, JSON.stringify({results: []}));
  await contentClient.getEntries({content_type: 'article'});
});

Fails with:

Error: Failed: Object {
  "code": "ECONNRESET",
  "columnNumber": undefined,
  "config": Object {
    "adapter": [Function httpAdapter],
    "baseURL": "https://cdn.contentful.com:443/spaces/my-space/environments/master",
    "data": undefined,
    "headers": [Object],
    "httpAgent": false,
    "httpsAgent": false,
    "maxContentLength": -1,
    "method": "get",
    "params": [Object],
    "paramsSerializer": [Function anonymous],
    "proxy": false,
    "timeout": 30000,
    "transformRequest": [Array],
    "transformResponse": [Array],
    "url": "https://cdn.contentful.com:443/spaces/my-space/environments/master/entries",
    "validateStatus": [Function validateStatus],
    "xsrfCookieName": "XSRF-TOKEN",
    "xsrfHeaderName": "X-XSRF-TOKEN",
  },
  "description": undefined,
  "fileName": undefined,
  "lineNumber": undefined,
  "message": "something awful happened",
  "name": undefined,
  "number": undefined,
  "stack": undefined,
}

Possible Solution

I'm not that familiar with Axios or contentful.js, but it looks like Axios ignores keys it does not know, like retryOnError, which means that retryOnError is not be available for retry-handler

It looks like this behaviour was changed in axios/axios#1395

Context

We are using beta version of contentful.js because of this issue: #272

Now, I have seen a few 429 and ECONNRESET errors on our site, and wondered why those are visible to our clients, when Contentful.js should be retrying those.

So, I decided to create nock-based test for checking out what happens, and based on that, retry does not seem to work.

Environment

• Contentful.js 7.1.0-beta0

Edit:
I did test this with 7.0.3, and it does not seem to have this problem.

[TomiTakussaari]

Related Axios issue: axios/axios#1718

[Khaledgarbaya]

Hi @TomiTakussaari,
We made sure to test all the cases in here. The reason that the SDK does not retry because it's an unknown status.
We rely on that to trigger the retry. Here is the code for that
Best,
Khaled

[TomiTakussaari]

@Khaledgarbaya Contentful-sdk seems to be still using Axios 0.18, which does not have this problem.

Problem is with Axios 0.19 beta (and above), as documented in axios/axios#1718.

This version is used in Contentful.js 7.1.0-beta0, that we have to use because of other problems with earlier, contentful bundled, version of Axios.

As I see it, because 'instance.defaults.retryOnError' on rate-limit.js will always be undefined with Axios 0.19, retry just won't work, regardless of status code.

There also does seem to be test for retrying on network error here (unknown status code): https://github.com/contentful/contentful-sdk-core/blob/master/test/unit/rate-limit-test.js#L89, and network error retry related code here: https://github.com/contentful/contentful-sdk-core/blob/master/lib/rate-limit.js#L20.

If those use cases are not supported, perhaps those should be removed to avoid any confusion?

[Khaledgarbaya]

Hey @TomiTakussaari contentful.js is using axios 0.18 also https://github.com/contentful/contentful.js/blob/master/package.json#L61.

Also, you are replying with an error without a status, which means that the response exist so it is not a connection error for the ratelimit logic

Best,
Khaled

[TomiTakussaari]

This version of contentful.js is not using axios 0.18: https://github.com/contentful/contentful.js/releases/tag/v7.1.0-beta0 &

contentful.js/package.json

Line 61 in c1b7286

"axios": "^0.19.0-beta.1",

I understand its a beta version, but I guess bugs in it should still be reported here ?
If not, we can of course also create customer support ticket if that is the preferred way.

Also, retry for this error does work with 7.0.3:

$ cat foo.test.ts

import nock from 'nock';
import * as contentful from 'contentful';

test('contentful client retries on network errors', async () => {
  const contentfulConfig = {
    accessToken: 'foobar',
    environment: 'master',
    space: 'my-space',
    retryOnError: true
  };
  const contentClient = contentful.createClient(contentfulConfig);
  nock('https://cdn.contentful.com')
    .get('/spaces/my-space/environments/master/entries?content_type=article')
    .replyWithError({'message': 'something awful happened', code: 'ECONNRESET'})
    .get('/spaces/my-space/environments/master/entries?content_type=article')
    .reply(200, JSON.stringify({results: []}));
  await contentClient.getEntries({content_type: 'article'});
});

$ yarn add contentful@7.1.0-beta0
$ npx jest foo
 FAIL  ./foo.test.ts
  ✕ contentful client retries on network errors (25ms)

  ● contentful client retries on network errors

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total

$ yarn add contentful@7.0.3
$ npx jest foo
  console.log node_modules/contentful/dist/contentful.node.js:2228
    [warning] Connection error occurred. Waiting for 2085 ms before retrying...

 PASS  ./foo.test.ts
  ✓ contentful client retries on network errors (2118ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total

[Khaledgarbaya]

Hey @TomiTakussaari,
We'll check this asap.

[Khaledgarbaya]

Hey @TomiTakussaari,
I think I found the affected line which breaks the retry logic.
I will try to fix it asap.

Best,
Khaled

[TomiTakussaari]

Any news @Khaledgarbaya ?

[sebbean]

havin timeout errors up the wazoo - is this related possibly?

1:13:05 PM: [warning] Connection error occurred. Waiting for 6264 ms before retrying...
1:13:05 PM: [21:13:05]  GEN ERR  /comedy/nick-offerman
1:13:05 PM: Error: timeout of 30000ms exceeded
1:13:05 PM:     at createError (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:895:15)
1:13:05 PM:     at Timeout.handleRequestTimeout [as _onTimeout] (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:318:16)
1:13:05 PM:     at ontimeout (timers.js:498:11)
1:13:05 PM:     at tryOnTimeout (timers.js:323:5)
1:13:05 PM:     at Timer.listOnTimeout (timers.js:290:5)
1:13:05 PM: [21:13:05]  GEN ERR  /digital/jimmy-tatro
1:13:05 PM: Error: timeout of 30000ms exceeded
1:13:05 PM:     at createError (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:895:15)
1:13:05 PM:     at Timeout.handleRequestTimeout [as _onTimeout] (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:318:16)
1:13:05 PM:     at ontimeout (timers.js:498:11)
1:13:05 PM:     at tryOnTimeout (timers.js:323:5)
1:13:05 PM:     at Timer.listOnTimeout (timers.js:290:5)
1:13:05 PM: [21:13:05]  GEN ERR  /digital/keke-palmer
1:13:05 PM: Error: timeout of 30000ms exceeded
1:13:05 PM:     at createError (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:895:15)
1:13:05 PM:     at Timeout.handleRequestTimeout [as _onTimeout] (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:318:16)
1:13:05 PM:     at ontimeout (timers.js:498:11)
1:13:05 PM:     at tryOnTimeout (timers.js:323:5)
1:13:05 PM:     at Timer.listOnTimeout (timers.js:290:5)
1:13:05 PM: [21:13:05]  GEN ERR  /digital
1:13:05 PM: Error: timeout of 30000ms exceeded
1:13:05 PM:     at createError (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:895:15)
1:13:06 PM:     at Timeout.handleRequestTimeout [as _onTimeout] (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:318:16)
1:13:06 PM:     at ontimeout (timers.js:498:11)
1:13:06 PM:     at tryOnTimeout (timers.js:323:5)
1:13:06 PM:     at Timer.listOnTimeout (timers.js:290:5)
1:13:06 PM: [21:13:05]  GEN ERR  /music
1:13:06 PM: Error: timeout of 30000ms exceeded
1:13:06 PM:     at createError (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:895:15)
1:13:06 PM:     at Timeout.handleRequestTimeout [as _onTimeout] (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:318:16)
1:13:06 PM:     at ontimeout (timers.js:498:11)
1:13:06 PM:     at tryOnTimeout (timers.js:323:5)
1:13:06 PM:     at Timer.listOnTimeout (timers.js:290:5)
1:13:06 PM: [21:13:05]  GEN ERR  /comedy
1:13:06 PM: Error: timeout of 30000ms exceeded
1:13:06 PM:     at createError (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:895:15)
1:13:06 PM:     at Timeout.handleRequestTimeout [as _onTimeout] (/opt/build/repo/node_modules/contentful/dist/contentful.node.js:318:16)

[Khaledgarbaya]

this should be fixed in the latest release, Please open a new issue if you still have the issue