Skip to content
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.

Sign up for GitHub

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

RpcService: Regard node-fetch errors as retriable #5298

Merged
merged 2 commits into from
Feb 12, 2025
Merged

RpcService: Regard node-fetch errors as retriable #5298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
449c7a0
RpcService: Regard node-fetch errors as retriable
mcmire Feb 6, 2025
7def565
Merge branch 'main' into include-node-fetch-errors-as-retriable
mcmire Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/network-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,13 @@
"@types/jest": "^27.4.1",
"@types/jest-when": "^2.7.3",
"@types/lodash": "^4.14.191",
"@types/node-fetch": "^2.6.12",
"deepmerge": "^4.2.2",
"jest": "^27.5.1",
"jest-when": "^3.4.2",
"lodash": "^4.17.21",
"nock": "^13.3.1",
"node-fetch": "^2.7.0",
"sinon": "^9.2.4",
"ts-jest": "^27.1.4",
"typedoc": "^0.24.8",
Expand Down
210 changes: 199 additions & 11 deletions packages/network-controller/src/rpc-service/rpc-service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@

import { rpcErrors } from '@metamask/rpc-errors';
import nock from 'nock';
import { FetchError } from 'node-fetch';
import { useFakeTimers } from 'sinon';
import type { SinonFakeTimers } from 'sinon';

import type { AbstractRpcService } from './abstract-rpc-service';
import { NETWORK_UNREACHABLE_ERRORS, RpcService } from './rpc-service';
import { RpcService } from './rpc-service';
import { DEFAULT_CIRCUIT_BREAK_DURATION } from '../../../controller-utils/src/create-service-policy';

describe('RpcService', () => {
Expand All @@ -22,10 +23,58 @@ describe('RpcService', () => {
});

describe('request', () => {
describe.each([...NETWORK_UNREACHABLE_ERRORS].slice(0, 1))(
`if making the request throws a "%s" error (as a "network unreachable" error)`,
(errorMessage) => {
const error = new TypeError(errorMessage);
// NOTE: Keep this list synced with CONNECTION_ERRORS
describe.each([
{
constructorName: 'TypeError',
message: 'network error',
},
{
constructorName: 'TypeError',
message: 'Failed to fetch',
},
{
constructorName: 'TypeError',
message: 'NetworkError when attempting to fetch resource.',
},
{
constructorName: 'TypeError',
message: 'The Internet connection appears to be offline.',
},
{
constructorName: 'TypeError',
message: 'Load failed',
},
{
constructorName: 'TypeError',
message: 'Network request failed',
},
{
constructorName: 'FetchError',
message: 'request to https://foo.com failed',
},
{
constructorName: 'TypeError',
message: 'fetch failed',
},
{
constructorName: 'TypeError',
message: 'terminated',
},
])(
`if making the request throws the $message error`,
({ constructorName, message }) => {
let error;
switch (constructorName) {
case 'FetchError':
error = new FetchError(message, 'system');
break;
case 'TypeError':
error = new TypeError(message);
break;
default:
throw new Error(`Unknown constructor ${constructorName}`);
}
testsForRetriableFetchErrors({
getClock: () => clock,
producedError: error,
Expand Down Expand Up @@ -59,6 +108,145 @@ describe('RpcService', () => {
},
);

describe('if the endpoint URL was not mocked via Nock', () => {
it('re-throws the error without retrying the request', async () => {
const service = new RpcService({
fetch,
btoa,
endpointUrl: 'https://rpc.example.chain',
});

const promise = service.request({
id: 1,
jsonrpc: '2.0',
method: 'eth_chainId',
params: [],
});
await expect(promise).rejects.toThrow('Nock: Disallowed net connect');
});

it('does not forward the request to a failover service if given one', async () => {
const failoverService = buildMockRpcService();
const service = new RpcService({
fetch,
btoa,
endpointUrl: 'https://rpc.example.chain',
failoverService,
});

const jsonRpcRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'eth_chainId',
params: [],
};
await ignoreRejection(service.request(jsonRpcRequest));
expect(failoverService.request).not.toHaveBeenCalled();
});

it('does not call onBreak', async () => {
const onBreakListener = jest.fn();
const service = new RpcService({
fetch,
btoa,
endpointUrl: 'https://rpc.example.chain',
});
service.onBreak(onBreakListener);

const promise = service.request({
id: 1,
jsonrpc: '2.0',
method: 'eth_chainId',
params: [],
});
await ignoreRejection(promise);
expect(onBreakListener).not.toHaveBeenCalled();
});
});

describe('if the endpoint URL was mocked via Nock, but not the RPC method', () => {
it('re-throws the error without retrying the request', async () => {
const endpointUrl = 'https://rpc.example.chain';
nock(endpointUrl)
.post('/', {
id: 1,
jsonrpc: '2.0',
method: 'eth_incorrectMethod',
params: [],
})
.reply(500);
const service = new RpcService({
fetch,
btoa,
endpointUrl,
});

const promise = service.request({
id: 1,
jsonrpc: '2.0',
method: 'eth_chainId',
params: [],
});
await expect(promise).rejects.toThrow('Nock: No match for request');
});

it('does not forward the request to a failover service if given one', async () => {
const endpointUrl = 'https://rpc.example.chain';
nock(endpointUrl)
.post('/', {
id: 1,
jsonrpc: '2.0',
method: 'eth_incorrectMethod',
params: [],
})
.reply(500);
const failoverService = buildMockRpcService();
const service = new RpcService({
fetch,
btoa,
endpointUrl,
failoverService,
});

const jsonRpcRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'eth_chainId',
params: [],
};
await ignoreRejection(service.request(jsonRpcRequest));
expect(failoverService.request).not.toHaveBeenCalled();
});

it('does not call onBreak', async () => {
const endpointUrl = 'https://rpc.example.chain';
nock(endpointUrl)
.post('/', {
id: 1,
jsonrpc: '2.0',
method: 'eth_incorrectMethod',
params: [],
})
.reply(500);
const onBreakListener = jest.fn();
const service = new RpcService({
fetch,
btoa,
endpointUrl,
});
service.onBreak(onBreakListener);

const promise = service.request({
id: 1,
jsonrpc: '2.0',
method: 'eth_chainId',
params: [],
});
await ignoreRejection(promise);
expect(onBreakListener).not.toHaveBeenCalled();
});
});

describe('if making the request throws an unknown error', () => {
it('re-throws the error without retrying the request', async () => {
const error = new Error('oops');
Expand Down Expand Up @@ -129,7 +317,7 @@ describe('RpcService', () => {
});

describe.each([503, 504])(
'if the endpoint consistently has a %d response',
'if the endpoint has a %d response',
(httpStatus) => {
testsForRetriableResponses({
getClock: () => clock,
Expand Down Expand Up @@ -191,7 +379,7 @@ describe('RpcService', () => {
const jsonRpcRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'eth_chainId',
method: 'eth_unknownMethod',
Copy link
Contributor Author

@mcmire mcmire Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I discovered while making these changes that these tests were actually incorrect but were not failing. Considering that they created Nock errors I decided to fix them at the same time.

params: [],
};
await ignoreRejection(service.request(jsonRpcRequest));
Expand Down Expand Up @@ -219,7 +407,7 @@ describe('RpcService', () => {
const promise = service.request({
id: 1,
jsonrpc: '2.0',
method: 'eth_chainId',
method: 'eth_unknownMethod',
params: [],
});
await ignoreRejection(promise);
Expand Down Expand Up @@ -259,7 +447,7 @@ describe('RpcService', () => {
.post('/', {
id: 1,
jsonrpc: '2.0',
method: 'eth_unknownMethod',
method: 'eth_chainId',
params: [],
})
.reply(429);
Expand Down Expand Up @@ -287,7 +475,7 @@ describe('RpcService', () => {
.post('/', {
id: 1,
jsonrpc: '2.0',
method: 'eth_unknownMethod',
method: 'eth_chainId',
params: [],
})
.reply(429);
Expand Down Expand Up @@ -355,7 +543,7 @@ describe('RpcService', () => {
.post('/', {
id: 1,
jsonrpc: '2.0',
method: 'eth_unknownMethod',
method: 'eth_chainId',
params: [],
})
.reply(500, {
Expand Down
Loading
Loading