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.

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

MV3: add retry logic to actions #15337

Merged
merged 43 commits into from
Sep 5, 2022
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
620c4ea
fix hanging RPC calls - trigger disconnect errors on unhandled calls.…
naugtur Aug 11, 2022
617610a
test: make metaRPCClientFactory actually assert results
naugtur Aug 11, 2022
68d0aae
fix lint issues
jpuri Aug 12, 2022
949a95d
MV3: add retry logic to actions
jpuri Jul 26, 2022
12b1504
More unit test coverage
jpuri Aug 5, 2022
1b6233d
Adding more test coverage
jpuri Aug 5, 2022
581ccfa
fix: unify queue with draining and connect up promise chains, fix tes…
naugtur Aug 8, 2022
b81a060
fix method names
jpuri Aug 8, 2022
0b4ddd9
lint fixes
jpuri Aug 8, 2022
3ef2439
test case fixes
jpuri Aug 8, 2022
5ffe662
fix: process actions queue in a sequence
naugtur Aug 9, 2022
4688316
implement dropping the queue with promise rejections, document exports
naugtur Aug 10, 2022
4342f4a
document a race condition with a test
naugtur Aug 10, 2022
3b4a007
revamp queue processing to eliminate the possibility of a race condition
naugtur Aug 10, 2022
220c9e4
fix hanging RPC calls - trigger disconnect errors on unhandled calls.…
naugtur Aug 11, 2022
fbd2b7c
parallelize actions when connected, keep the queue sequential on reco…
naugtur Aug 12, 2022
29d8a0a
fix
jpuri Aug 12, 2022
fc9d189
fix
jpuri Aug 12, 2022
30094ba
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
jpuri Aug 16, 2022
33f1a1d
merge
jpuri Aug 16, 2022
9a76a87
fix lint issue
jpuri Aug 16, 2022
e48a245
fix
jpuri Aug 16, 2022
acd5f2e
fix
jpuri Aug 16, 2022
5c7da1f
Merge branch 'develop' into mv3_action_retry
jpuri Aug 16, 2022
85e5bb1
merge
jpuri Aug 16, 2022
721957d
Merge branch 'mv3_action_retry' of github.com:MetaMask/metamask-exten…
jpuri Aug 16, 2022
23ed652
fix
jpuri Aug 16, 2022
080cd36
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
jpuri Aug 17, 2022
baa8ef0
Passing actionId as last argument to background method call
jpuri Aug 22, 2022
7000616
Renaming method
jpuri Aug 22, 2022
3b11f94
fix
jpuri Aug 22, 2022
0c19e3e
Merge branch 'develop' into mv3_action_retry
jpuri Aug 23, 2022
8161b91
Update ui/store/action-queue/index.js
jpuri Aug 23, 2022
16022ad
Merge branch 'develop' into mv3_action_retry
jpuri Aug 23, 2022
11388b4
lint fix
jpuri Aug 24, 2022
19af2f4
fix
jpuri Aug 24, 2022
b67442c
fix
jpuri Aug 24, 2022
678f4af
Merge branch 'develop' into mv3_action_retry
jpuri Aug 24, 2022
85c7075
Merge branch 'develop' into mv3_action_retry
jpuri Aug 25, 2022
825205b
Update ui/store/action-queue/index.js
jpuri Aug 25, 2022
650a94c
fix
jpuri Aug 25, 2022
a17b32a
Merge branch 'develop' into mv3_action_retry
jpuri Aug 26, 2022
cebe284
Merge branch 'develop' of github.com:MetaMask/metamask-extension into…
jpuri Sep 5, 2022
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: 1 addition & 1 deletion .storybook/preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import MetaMetricsProviderStorybook from './metametrics';
import testData from './test-data.js';
import { Router } from 'react-router-dom';
import { createBrowserHistory } from 'history';
import { _setBackgroundConnection } from '../ui/store/actions';
import { _setBackgroundConnection } from '../ui/store/action-queue';
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: my brain sees underscore before a variable/method and it thinks private variable/method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would avoid changing existing method in this PR.

import MetaMaskStorybookTheme from './metamask-storybook-theme';
import addons from '@storybook/addons';

Expand Down
2 changes: 2 additions & 0 deletions app/scripts/ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ async function start() {
const messageListener = (message) => {
if (message?.name === 'CONNECTION_READY') {
if (isUIInitialised) {
// Currently when service worker is revived we create new streams
// in later version we might try to improve it by reviving same streams.
updateUiStreams();
} else {
initializeUiWithTab(activeTab);
Expand Down
4 changes: 2 additions & 2 deletions test/jest/background.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as actions from '../../ui/store/actions';
import { _setBackgroundConnection } from '../../ui/store/action-queue';

export const setBackgroundConnection = (backgroundConnection = {}) => {
actions._setBackgroundConnection(backgroundConnection);
_setBackgroundConnection(backgroundConnection);
};
3 changes: 2 additions & 1 deletion ui/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
} from './ducks/metamask/metamask';
import Root from './pages';
import txHelper from './helpers/utils/tx-helper';
import { _setBackgroundConnection } from './store/action-queue';

log.setLevel(global.METAMASK_DEBUG ? 'debug' : 'warn');

Expand All @@ -39,7 +40,7 @@ let reduxStore;
* @param backgroundConnection - connection object to background
*/
export const updateBackgroundConnection = (backgroundConnection) => {
actions._setBackgroundConnection(backgroundConnection);
_setBackgroundConnection(backgroundConnection);
backgroundConnection.onNotification((data) => {
if (data.method === 'sendUpdate') {
reduxStore.dispatch(actions.updateMetamaskState(data.params[0]));
Expand Down
200 changes: 200 additions & 0 deletions ui/store/action-queue/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
import pify from 'pify';
naugtur marked this conversation as resolved.
Show resolved Hide resolved
import { isManifestV3 } from '../../../shared/modules/mv3.utils';

// // A simplified pify maybe?
// function pify(apiObject) {
// return Object.keys(apiObject).reduce((promisifiedAPI, key) => {
// if (apiObject[key].apply) { // depending on our browser support we might use a nicer check for functions here
// promisifiedAPI[key] = function (...args) {
// return new Promise((resolve, reject) => {
// return apiObject[key](
// ...args,
// (err, result) => {
// if (err) {
// reject(err);
// } else {
// resolve(result);
// }
// },
// );
// });
// };
// }
// return promisifiedAPI;
// }, {});
// }

let background = null;
let promisifiedBackground = null;

const actionRetryQueue = [];

function failQueue() {
actionRetryQueue.forEach(({ reject }) =>
reject(
Error('Background operation cancelled while waiting for connection.'),
),
);
}

/**
* Drops the entire actions queue. Rejects all actions in the queue unless silently==true
* Does not affect the single action that is currently being processed.
*
* @param {boolean} [silently]
*/
export function dropQueue(silently) {
if (!silently) {
failQueue();
}
actionRetryQueue.length = 0;
}

// add action to queue
const executeActionOrAddToRetryQueue = (item) => {
if (actionRetryQueue.some((act) => act.actionId === item.actionId)) {
return;
}

if (background.connectionStream.readable) {
executeAction({
action: item,
disconnectSideeffect: () => actionRetryQueue.push(item),
});
} else {
actionRetryQueue.push(item);
}
};

/**
* Promise-style call to background method
* In MV2: invokes promisifiedBackground method directly.
* In MV3: action is added to retry queue, along with resolve handler to be executed on completion,
* the queue is then immediately processed if background connection is available.
* On completion (successful or error) the action is removed from the retry queue.
*
* @param {string} method - name of the background method
* @param {Array} [args] - arguments to that method, if any
* @param {any} [actionId] - if an action with the === same id is submitted, it'll be ignored if already in queue waiting for a retry.
* @returns {Promise}
*/
export function submitRequestToBackground(
method,
args = [],
actionId = Date.now() + Math.random(), // current date is not guaranteed to be unique
digiwand marked this conversation as resolved.
Show resolved Hide resolved
) {
if (isManifestV3) {
return new Promise((resolve, reject) => {
executeActionOrAddToRetryQueue({
actionId,
request: { method, args },
resolve,
reject,
});
});
}
return promisifiedBackground[method](...args);
}

/**
* Callback-style call to background method
* In MV2: invokes promisifiedBackground method directly.
* In MV3: action is added to retry queue, along with resolve handler to be executed on completion,
* the queue is then immediately processed if background connection is available.
* On completion (successful or error) the action is removed from the retry queue.
*
* @param {string} method - name of the background method
* @param {Array} [args] - arguments to that method, if any
* @param callback - Node style (error, result) callback for finishing the operation
* @param {any} [actionId] - if an action with the === same id is submitted, it'll be ignored if already in queue.
*/
export const callBackgroundMethod = (
method,
args = [],
callback,
actionId = Date.now() + Math.random(), // current date is not guaranteed to be unique
digiwand marked this conversation as resolved.
Show resolved Hide resolved
) => {
if (isManifestV3) {
const resolve = (value) => callback(null, value);
const reject = (err) => callback(err);
executeActionOrAddToRetryQueue({
actionId,
request: { method, args },
resolve,
reject,
});
} else {
background[method](...args, callback);
}
};

async function executeAction({ action, disconnectSideeffect }) {
const {
request: { method, args },
resolve,
reject,
} = action;
try {
resolve(await promisifiedBackground[method](...args));
} catch (err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@naugtur , I have added actionId here as the last argument, it is useful to make action call idempotent in some scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

It crosses abstraction boundaries. Now all background functions have to understand they're being used that way.

  • How far does it reach? Is it used in the RPC client or in the background worker?
  • Why is it only added in one place and not the others? Seems like an oversight.

IMHO, if we want to add it, it'd need to be the first argument and be made mandatory, otherwise it'll hurt us later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used it here https://github.com/MetaMask/metamask-extension/pull/15667/files#diff-675c437262d906486cf5436c0b64535347286cc4d1081dbd863fb45b8a01b81aR755

I kept it last argument so that methods deal with it only if they need to. If we do not want to pass it to all background methods, I can pass it specifically where needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Intuition tells me it's a footgun.

Current change visible here sends it or doesn't send it depending on the current state - you only added it in the queue processing but it's not defined if connection was available.
So it only guarantees idempotence across multiple retries, but not between the initial call and first retry

Copy link
Contributor Author

@jpuri jpuri Aug 22, 2022

Choose a reason for hiding this comment

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

I changed it to pass actionId only when needed.

if (
background.DisconnectError && // necessary to not break compatibility with background stubs or non-default implementations
err instanceof background.DisconnectError
) {
disconnectSideeffect(action);
} else {
reject(err);
}
}
}

let processingQueue = false;

// Clears list of pending action in actionRetryQueue
// The results of background calls are wired up to the original promises that's been returned
// The first method on the queue gets called synchronously to make testing and reasoning about
// a single request to an open connection easier.
async function processActionRetryQueue() {
if (processingQueue) {
return;
}
processingQueue = true;
try {
while (
background.connectionStream.readable &&
actionRetryQueue.length > 0
) {
// If background disconnects and fails the action, the next one will not be taken off the queue.
// Retrying an action that failed because of connection loss while it was processing is not supported.
const item = actionRetryQueue.shift();
await executeAction({
action: item,
disconnectSideeffect: () => actionRetryQueue.unshift(item),
});
}
} catch (e) {
// error in the queue mechanism itself, the action was malformed
console.error(e);
}
processingQueue = false;
}

/**
* Sets/replaces the background connection reference
* Under MV3 it also triggers queue processing if the new background is connected
*
* @param {*} backgroundConnection
*/
export async function _setBackgroundConnection(backgroundConnection) {
background = backgroundConnection;
promisifiedBackground = pify(background);
if (isManifestV3) {
if (processingQueue) {
console.warn(
'_setBackgroundConnection called while a queue was processing and not disconnected yet',
);
}
// Process all actions collected while connection stream was not available.
processActionRetryQueue();
}
}
Loading