feat: add sequential task queue to prevent parallel overload#2010
Open
AMIRKHANEF wants to merge 1 commit intoPolkaGate:mainfrom
Open
feat: add sequential task queue to prevent parallel overload#2010AMIRKHANEF wants to merge 1 commit intoPolkaGate:mainfrom
AMIRKHANEF wants to merge 1 commit intoPolkaGate:mainfrom
Conversation
Contributor
WalkthroughThe SharedWorker message handler is refactored to process incoming requests sequentially through a new RequestQueue class instead of handling them in parallel. Error handling is enhanced with per-task try/catch blocks, and conditional logic for asset fetching is updated to handle undefined cases explicitly. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SW as SharedWorker
participant Queue as RequestQueue
participant Handler as Handler Functions
rect rgb(240, 248, 255)
Note over SW,Queue: Old: Parallel Processing
Client->>SW: Message 1
Client->>SW: Message 2
SW->>Handler: Request 1 (async)
SW->>Handler: Request 2 (async)
Handler-->>SW: Response 1
Handler-->>SW: Response 2
end
rect rgb(240, 255, 240)
Note over SW,Queue: New: Sequential Queue Processing
Client->>SW: Message 1
SW->>Queue: add(async task 1)
Client->>SW: Message 2
SW->>Queue: add(async task 2)
Queue->>Handler: Execute task 1
Handler-->>Queue: Complete ✓
Queue->>Handler: Execute task 2
Handler-->>Queue: Complete ✓
Queue-->>SW: Results
SW-->>Client: Response 1
SW-->>Client: Response 2
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/util/workers/sharedWorker.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/util/workers/sharedWorker.js (5)
packages/extension-polkagate/src/util/constants.ts (1)
FETCHING_ASSETS_FUNCTION_NAMES(337-341)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnRelayChain.js (1)
getAssetOnRelayChain(15-58)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnMultiAssetChain.js (1)
getAssetOnMultiAssetChain(21-110)packages/extension-polkagate/src/util/workers/shared-helpers/getAssetOnAssetHub.js (1)
getAssetOnAssetHub(16-46)packages/extension-polkagate/src/util/workers/shared-helpers/getValidatorsInformation.js (1)
getValidatorsInformation(62-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
Comment on lines
+86
to
99
| case FETCHING_ASSETS_FUNCTION_NAMES.MULTI_ASSET: { | ||
| const assetsToBeFetched = assetsChains[parameters.chainName]; | ||
|
|
||
| case FETCHING_ASSETS_FUNCTION_NAMES.ASSET_HUB: { | ||
| if (!parameters.assetsToBeFetched) { | ||
| console.warn('getAssetOnAssetHub: No assets to be fetched on, but just Native Token'); | ||
| /** if assetsToBeFetched === undefined then we don't fetch assets by default at first, but will fetch them on-demand later in account details page*/ | ||
| if (!assetsToBeFetched) { | ||
| console.info(`Shared worker, getAssetOnMultiAssetChain: No assets to be fetched on ${parameters.chainName}`); | ||
| port.postMessage(JSON.stringify({ functionName })); | ||
|
|
||
| parameters.assetsToBeFetched = []; | ||
| return; | ||
| } | ||
|
|
||
| await getAssetOnMultiAssetChain(assetsToBeFetched, ...params, port); | ||
| break; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix the MULTI_ASSET argument order
params still contains parameters.assetsToBeFetched, so when you prepend the new assetsChains lookup and spread ...params, getAssetOnMultiAssetChain now receives the caller’s assetsToBeFetched array where it expects chainName. That turns into a runtime failure (getChainEndpoints gets an array instead of a string) and breaks the “maintain compatibility with existing message handling logic” requirement. Please destructure the fields you actually need and pass them explicitly to keep the signature aligned.
Apply this diff:
- case FETCHING_ASSETS_FUNCTION_NAMES.MULTI_ASSET: {
- const assetsToBeFetched = assetsChains[parameters.chainName];
+ case FETCHING_ASSETS_FUNCTION_NAMES.MULTI_ASSET: {
+ const { addresses, chainName, userAddedEndpoints } = parameters;
+ const assetsToBeFetched = assetsChains[chainName];
/** if assetsToBeFetched === undefined then we don't fetch assets by default at first, but will fetch them on-demand later in account details page*/
if (!assetsToBeFetched) {
- console.info(`Shared worker, getAssetOnMultiAssetChain: No assets to be fetched on ${parameters.chainName}`);
+ console.info(`Shared worker, getAssetOnMultiAssetChain: No assets to be fetched on ${chainName}`);
port.postMessage(JSON.stringify({ functionName }));
return;
}
- await getAssetOnMultiAssetChain(assetsToBeFetched, ...params, port);
+ await getAssetOnMultiAssetChain(
+ assetsToBeFetched,
+ addresses,
+ chainName,
+ userAddedEndpoints,
+ port
+ );
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case FETCHING_ASSETS_FUNCTION_NAMES.MULTI_ASSET: { | |
| const assetsToBeFetched = assetsChains[parameters.chainName]; | |
| case FETCHING_ASSETS_FUNCTION_NAMES.ASSET_HUB: { | |
| if (!parameters.assetsToBeFetched) { | |
| console.warn('getAssetOnAssetHub: No assets to be fetched on, but just Native Token'); | |
| /** if assetsToBeFetched === undefined then we don't fetch assets by default at first, but will fetch them on-demand later in account details page*/ | |
| if (!assetsToBeFetched) { | |
| console.info(`Shared worker, getAssetOnMultiAssetChain: No assets to be fetched on ${parameters.chainName}`); | |
| port.postMessage(JSON.stringify({ functionName })); | |
| parameters.assetsToBeFetched = []; | |
| return; | |
| } | |
| await getAssetOnMultiAssetChain(assetsToBeFetched, ...params, port); | |
| break; | |
| } | |
| case FETCHING_ASSETS_FUNCTION_NAMES.MULTI_ASSET: { | |
| const { addresses, chainName, userAddedEndpoints } = parameters; | |
| const assetsToBeFetched = assetsChains[chainName]; | |
| /** if assetsToBeFetched === undefined then we don't fetch assets by default at first, but will fetch them on-demand later in account details page*/ | |
| if (!assetsToBeFetched) { | |
| console.info(`Shared worker, getAssetOnMultiAssetChain: No assets to be fetched on ${chainName}`); | |
| port.postMessage(JSON.stringify({ functionName })); | |
| return; | |
| } | |
| await getAssetOnMultiAssetChain( | |
| assetsToBeFetched, | |
| addresses, | |
| chainName, | |
| userAddedEndpoints, | |
| port | |
| ); | |
| break; | |
| } |
🤖 Prompt for AI Agents
In packages/extension-polkagate/src/util/workers/sharedWorker.js around lines 86
to 99, the call to getAssetOnMultiAssetChain spreads params which still contains
the original assetsToBeFetched, causing the function to receive arguments in the
wrong order (chainName vs assets array). Destructure the incoming message to
extract exactly the required fields (e.g., chainName and any other params like
accountId or asset identifiers), replace the spread ...params with explicit
arguments so you call getAssetOnMultiAssetChain(chainName, assetsToBeFetched, /*
other explicit args in correct order */, port), and remove or update any
redundant parameters to preserve the original function signature and avoid
passing the caller’s assets array as the chainName.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces a sequential task queue for the SharedWorker to avoid simultaneous network requests that cause performance bottlenecks.
Closes #2009
Summary by CodeRabbit
Bug Fixes
Refactor