Skip to content

Commit

Permalink
Fix NetworkController to handle clearing of messenger
Browse files Browse the repository at this point in the history 
If the network controller is in the process of executing the
`lookupNetwork` step and the messenger is cleared of subscriptions, then
it may throw an error that the `networkDidChange` subscription is
missing. This happens in Mobile when it destroys the engine. There are
actually two places where `lookupNetwork` subscribes to
`networkDidChange` and then unsubscribes, but the aforementioned error
is only replicable with one of them. However, we handle both cases just
in case.
  • Loading branch information
@mcmire
mcmire committed Jan 8, 2025
1 parent 7c38c24 commit 8590275
Show file tree
Hide file tree
Showing 3 changed files with 272 additions and 8 deletions.
5 changes: 5 additions & 0 deletions packages/network-controller/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Bump `@metamask/base-controller` from `^7.0.0` to `^7.1.0` ([#5079](https://github.com/MetaMask/core/pull/5079))

### Fixed

- Fix `lookupNetwork` so that it will no longer throw an error if `networkDidChange` subscriptions have been removed before it returns
- This error could occur if the NetworkController's messenger is cleared of subscriptions, as in a "destroy" step.

## [22.1.1]

### Changed
Expand Down
47 changes: 39 additions & 8 deletions packages/network-controller/src/NetworkController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1300,10 +1300,30 @@ export class NetworkController extends BaseController<
let networkChanged = false;
const listener = () => {
networkChanged = true;
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);
try {
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);
} catch (error) {
// In theory, this `catch` should not be necessary given that this error
// would occur "inside" of the call to `#determineEIP1559Compatibility`
// below and so it should be caught by the `try`/`catch` below (it is
// impossible to reproduce in tests for that reason). However, somehow
// it occurs within Mobile and so we have to add our own `try`/`catch`
// here.
/* istanbul ignore next */
if (
!(error instanceof Error) ||
error.message !==
'Subscription not found for event: NetworkController:networkDidChange'
) {
// Again, this error should not happen and is impossible to reproduce
// in tests.
/* istanbul ignore next */
throw error;
}
}
};
this.messagingSystem.subscribe(
'NetworkController:networkDidChange',
Expand Down Expand Up @@ -1370,10 +1390,21 @@ export class NetworkController extends BaseController<
// in the process of being called, so we don't need to go further.
return;
}
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);

try {
this.messagingSystem.unsubscribe(
'NetworkController:networkDidChange',
listener,
);
} catch (error) {
if (
!(error instanceof Error) ||
error.message !==
'Subscription not found for event: NetworkController:networkDidChange'
) {
throw error;
}
}

this.update((state) => {
const meta = state.networksMetadata[state.selectedNetworkClientId];
Expand Down
228 changes: 228 additions & 0 deletions packages/network-controller/tests/NetworkController.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1579,6 +1579,109 @@ describe('NetworkController', () => {
});
});

describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => {
it('does not throw an error', async () => {
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: infuraNetworkType,
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: ChainId[infuraNetworkType],
infuraProjectId,
network: infuraNetworkType,
ticker: NetworksTicker[infuraNetworkType],
type: NetworkClientType.Infura,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
messenger.clearSubscriptions();
expect(await lookupNetworkPromise).toBeUndefined();
},
);
});
});

describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
it('re-throws the error', async () => {
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: infuraNetworkType,
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: ChainId[infuraNetworkType],
infuraProjectId,
network: infuraNetworkType,
ticker: NetworksTicker[infuraNetworkType],
type: NetworkClientType.Infura,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
const error = new Error('oops');
jest
.spyOn(messenger, 'unsubscribe')
.mockImplementation((eventType) => {
if (eventType === 'NetworkController:networkDidChange') {
throw error;
}
});
await expect(lookupNetworkPromise).rejects.toThrow(error);
},
);
});
});

lookupNetworkTests({
expectedNetworkClientType: NetworkClientType.Infura,
initialState: {
Expand Down Expand Up @@ -1889,6 +1992,131 @@ describe('NetworkController', () => {
});
});

describe('if all subscriptions are removed from the messenger before the call to lookupNetwork completes', () => {
it('does not throw an error', async () => {

Check warning on line 1996 in packages/network-controller/tests/NetworkController.test.ts

View workflow job for this annotation

GitHub Actions / Lint, build, and test / Lint (20.x) 

Test has no assertions
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',
networkConfigurationsByChainId: {
'0x1337': buildCustomNetworkConfiguration({
chainId: '0x1337',
nativeCurrency: 'TEST',
rpcEndpoints: [
buildCustomRpcEndpoint({
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
url: 'https://test.network',
}),
],
}),
},
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: '0x1337',
rpcUrl: 'https://test.network',
ticker: 'TEST',
type: NetworkClientType.Custom,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
messenger.clearSubscriptions();
await lookupNetworkPromise;
},
);
});
});

describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
it('re-throws the error', async () => {
const infuraProjectId = 'some-infura-project-id';

await withController(
{
state: {
selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',
networkConfigurationsByChainId: {
'0x1337': buildCustomNetworkConfiguration({
chainId: '0x1337',
nativeCurrency: 'TEST',
rpcEndpoints: [
buildCustomRpcEndpoint({
networkClientId: 'AAAA-AAAA-AAAA-AAAA',
url: 'https://test.network',
}),
],
}),
},
},
infuraProjectId,
},
async ({ controller, messenger }) => {
const fakeProvider = buildFakeProvider([
// Called during provider initialization
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
// Called via `lookupNetwork` directly
{
request: {
method: 'eth_getBlockByNumber',
},
response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
},
]);
const fakeNetworkClient = buildFakeClient(fakeProvider);
mockCreateNetworkClient()
.calledWith({
chainId: '0x1337',
rpcUrl: 'https://test.network',
ticker: 'TEST',
type: NetworkClientType.Custom,
})
.mockReturnValue(fakeNetworkClient);
await controller.initializeProvider();

const lookupNetworkPromise = controller.lookupNetwork();
const error = new Error('oops');
jest
.spyOn(messenger, 'unsubscribe')
.mockImplementation((eventType) => {
if (eventType === 'NetworkController:networkDidChange') {
throw error;
}
});
await expect(lookupNetworkPromise).rejects.toThrow(error);
},
);
});
});

lookupNetworkTests({
expectedNetworkClientType: NetworkClientType.Custom,
initialState: {
Expand Down

0 comments on commit 8590275

Please sign in to comment.