Skip to content
This repository was archived by the owner on Nov 9, 2023. It is now read-only.

Conversation

@jpuri
Copy link
Contributor

@jpuri jpuri commented Oct 14, 2022

The PR has following changes, in context of DAPP action replay for MV3:

  1. add retry count to request, max retries are 3
  2. requests are not retried if request.id is undefined, undefined is valid value of request.id.
  3. error is not throw is a response is encountered for which there is no corresponding request - reason for doing this change is at times I found that due to service worker restart requests are sent to background multiple times and there are multiple responses, as first response matched request is removed from queue and then as second response reaches there is no corresponding request and it throws exception

@jpuri jpuri force-pushed the request_retry_count branch from 1e4d971 to 406a395 Compare October 14, 2022 14:32
@jpuri jpuri marked this pull request as ready for review October 21, 2022 09:09
@jpuri jpuri requested a review from a team as a code owner October 21, 2022 09:09
@jpuri jpuri requested review from Gudahtt and naugtur October 21, 2022 09:09
@jpuri
Copy link
Contributor Author

jpuri commented Oct 21, 2022

The build is red as PR is marginally missing the threshold for test coverage. I have test covered the functionality already.

if (!context) {
throw new Error(`StreamMiddleware - Unknown response id "${res.id}"`);
console.warn(`StreamMiddleware - Unknown response id "${res.id}"`);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

(could) Document why this is just a warning, not an error with a long comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

naugtur
naugtur previously approved these changes Oct 24, 2022
@naugtur
Copy link
Contributor

naugtur commented Oct 24, 2022

Question: Should we respond with an error instead of failing silently if we run out of retries? I believe we should.

Co-authored-by: Zbyszek Tenerowicz <[email protected]>
@jpuri jpuri requested a review from naugtur October 25, 2022 09:33
@jpuri jpuri requested a review from Gudahtt October 26, 2022 08:25
@jpuri jpuri requested a review from Gudahtt October 27, 2022 07:26
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! One remaining lint error but then I can approve

@jpuri jpuri requested a review from Gudahtt October 27, 2022 18:38
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@jpuri jpuri merged commit 4f97224 into main Oct 28, 2022
@jpuri jpuri deleted the request_retry_count branch October 28, 2022 08:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants