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

Fix potential delay in sending out requests from the rust SDK #3717

Merged
merged 8 commits into from
Sep 18, 2023

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Sep 8, 2023

There was a potential race which could cause us to be very slow to send out pending HTTP requests, particularly when handling a user verification. Add some resilience to make sure we handle it correctly.

Based on #3716


This change is marked as an internal change (Task), so will not be included in the changelog.

This now happens as a side-effect of importing the keys.
There was a potential race which could cause us to be very slow to send out
pending HTTP requests, particularly when handling a user verification. Add some
resiliece to make sure we handle it correctly.
@@ -1304,6 +1311,8 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
throw new Error("missing roomId in the event");
}

logger.debug(`Incoming verification event ${event.getId()} type ${event.getType()} from ${event.getSender()}`);
Copy link
Member Author

Choose a reason for hiding this comment

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

this was just some really useful debugging which I'm throwing in while I'm in the area.

@richvdh richvdh marked this pull request as ready for review September 8, 2023 02:30
@richvdh richvdh requested a review from a team as a code owner September 8, 2023 02:30
@richvdh richvdh requested review from dbkr and weeman1337 and removed request for a team September 8, 2023 02:30
Base automatically changed from rav/element-r/user_trust_status_changed to develop September 8, 2023 04:52
@github-actions github-actions bot requested a review from a team as a code owner September 8, 2023 04:52
@github-actions github-actions bot requested a review from andybalaam September 8, 2023 04:52
@andybalaam
Copy link
Member

Updated from develop to make the diff clearer, since the based-on PR is merged. (Hope that's ok)

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks sensible and as described. Any way to test this?

return;
}
this.outgoingRequestLoopInner().catch((e) => {
Copy link
Member

Choose a reason for hiding this comment

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

Worth a comment saying we are deliberately not awaiting this here?

Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I've also tested device verification on the deployment. Works.

I suppose that writing tests for this edge case is not easily doable?

this.outgoingRequestLoopRunning = true;
try {
while (!this.stopped) {
this.outgoingRequestLoopOneMoreLoop = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another short comment here? Because if you oversee the await one may think that this.outgoingRequestLoopOneMoreLoop is always false in the if below.

@richvdh
Copy link
Member Author

richvdh commented Sep 18, 2023

I've added some comments, and a unit test. A proper integration test is a pain in the butt. but the problem certainly showed up in the react-sdk cypress tests so hopefully that will do Edit: this isn't true, unfortunately. It just caused an intermittent failure in the cypress tests in matrix-org/matrix-react-sdk#11364. It remains true that a proper integration test is a pain in the butt.

Merged via the queue into develop with commit 5e542b3 Sep 18, 2023
20 checks passed
@richvdh richvdh deleted the rav/element-r/fix_outgoing_queueu branch September 18, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants