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.

Sign up for GitHub

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

Jump to bottom

Excessive calls to GET /relations and GET /event, with MSC3981 enabled and working #25395

Closed
Tracked by #24392
turt2live opened this issue May 17, 2023 · 17 comments · Fixed by matrix-org/matrix-js-sdk#3541
Closed
Tracked by #24392

Excessive calls to GET /relations and GET /event, with MSC3981 enabled and working #25395

turt2live opened this issue May 17, 2023 · 17 comments · Fixed by matrix-org/matrix-js-sdk#3541
Assignees
@t3chguy
Labels
O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect

Comments

@turt2live
Copy link
Member

Steps to reproduce

  1. Restart app
  2. Click around a couple rooms
  3. Enjoy the flood of http requests

Outcome

What did you expect?

For the app to start up quickly, quietly (this bug causes my CPU fans to be fast-moving), and without duplicating requests.

What happened instead?

There's tens of thousands of requests, so haven't identified a pattern yet. A few example URLs are:

  • https://t2l.io/_matrix/client/v1/rooms/!REDACTED:matrix.org/relations/$REDACTED/m.replace/m.room.encrypted?limit=1
  • https://t2l.io/_matrix/client/v1/rooms/!REDACTED:element.io/relations/$REDACTED/m.replace/m.room.encrypted?limit=1
  • https://t2l.io/_matrix/client/r0/rooms/!REDACTED:element.io/event/$REDACTED
  • etc

Specifically on the GET /event calls, many appear to be duplicated:
image

I was told ages ago that MSC3981 would fix this, but it hasn't (or the scope of the MSC changed between 6 months ago and now).

Operating system

Windows 11

Application version

Element Nightly version: 0.0.1-nightly.2023051701 Olm version: 3.2.14

How did you install the app?

The Internet

Homeserver

t2l.io

Will you send logs?

Yes
@turt2live
Copy link
Member Author

This can cause /sync to be suffocated of resources, making it impossible to actually keep up to date for upwards of 30-40 minutes (in my case)

@clokep
Copy link

clokep commented May 18, 2023

I was told ages ago that MSC3981 would fix this, but it hasn't (or the scope of the MSC changed between 6 months ago and now).

I thought MSC3925 was meant to fix this, not MSC3981?

@turt2live
Copy link
Member Author

I've just turned on MSC3981 support, and while it's reduced a lot of the /relations spam, I'm still seeing plenty of duplicated requests for GET /event.

@turt2live
Copy link
Member Author

I spoke too soon: it's now spamming /relations too (same request as before).

@justjanne
Copy link
Contributor

The implementation of MSC 3981 in element-web was actually slightly faulty, as it still did the old-style /relations requests (though ignored the results). This should be fixed now.

@turt2live
Copy link
Member Author

/relations is indeed fixed (assuming server support is advertised). GET /event is 64% of all requests my client makes, though (as I understand it though, the /relations fix wouldn't have fixed this particular one).

@andybalaam andybalaam added S-Major Severely degrades major functionality or product features, with no satisfactory workaround O-Occasional Affects or can be seen by some users regularly or most users rarely labels May 23, 2023
@andybalaam
Copy link
Contributor

@turt2live I assume your server supports MSC3981? Element Web won't use it unless the server advertises support for it.

@turt2live
Copy link
Member Author

Yes, enabling MSC3981 support stopped the /relations spam, per above.

@Johennes Johennes mentioned this issue Jun 15, 2023
Open
@kerryarchibald kerryarchibald mentioned this issue Jul 3, 2023
Closed
@richvdh
Copy link
Member

richvdh commented Jul 3, 2023

Excessive calls to GET /relations and GET /event, with MSC3981 enabled and working [my emphasis]
Yes, enabling MSC3981 support stopped the /relations spam, per above.

These two statements seem to be in conflict. Which is correct?

In any case, I experienced much the same (cf #25705), and it doesn't seem to be making a /relations request for every event in the thread, so how would MSC3981 help?

@richvdh
Copy link
Member

richvdh commented Jul 3, 2023

I think this is mis-triaged. It's basically a show-stopping DoS which happens every time you reload the app.

@richvdh richvdh added O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience and removed O-Occasional Affects or can be seen by some users regularly or most users rarely labels Jul 3, 2023
@Johennes
Copy link
Contributor

Johennes commented Jul 4, 2023

The /event calls might stem from matrix-org/matrix-js-sdk#3427 which compensates for the absence of server-side support for matrix-org/matrix-spec-proposals#4023.

@richvdh
Copy link
Member

richvdh commented Jul 4, 2023

Even if we have to compensate for lack of a server-side feature, can we do so in a way that doesn't require requesting the same event hundreds of times?

@t3chguy
Copy link
Member

t3chguy commented Jul 4, 2023

matrix-org/matrix-js-sdk#3530 was a dropped attempt at resolving this

@Johennes
Copy link
Contributor

Johennes commented Jul 4, 2023

@t3chguy you said in chat earlier that you don't think this stems from matrix-org/matrix-js-sdk#3427. I didn't fully understand what you meant by the following though:

That's the behaviour of constructing a thread without the necessary MSCs iirc

Could you expand on that?

@t3chguy
Copy link
Member

t3chguy commented Jul 4, 2023

@Johennes when constructing a Thread object, we use fetchRoomEvent (/event/ API) to load the event if it isn't already locally known. I may have been mistaken wrt to any relation to MSCs here, it may be unavoidable.

@Johennes
Copy link
Contributor

Johennes commented Jul 4, 2023

@t3chguy and I discussed some more in DM. The O- and S- levels put this below what the WAT would pick up as a fire currently. However, there is uncertainty about whether or not this is a recent regression. @t3chguy will try to do a time-boxed investigation into this tomorrow to help inform further decisions.

@turt2live
Copy link
Member Author

tbh, if the fix were a promise cache on getEvent, I'd consider this fixed enough for now. I wouldn't expect that to take much more than a day (week with review).

@t3chguy t3chguy self-assigned this Jul 5, 2023
@t3chguy t3chguy mentioned this issue Jul 5, 2023
Merged
su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this issue Feb 24, 2024
@su-ex
Merge tag 'v27.0.0' into sc
9bbc329 
* Drop support for Node 16 ([\matrix-org#3533](matrix-org#3533)).
* Improve types around login, registration, UIA and identity servers ([\matrix-org#3537](matrix-org#3537)).
* **The Browserify artifact is being deprecated, scheduled for removal in the October 10th release cycle. (matrix-org#3189)**
* Simplify `MatrixClient::setPowerLevel` API ([\matrix-org#3570](matrix-org#3570)). Fixes element-hq/element-web#13900 and matrix-org#1844.
* Deprecate `VerificationRequest.getQRCodeBytes` and replace it with the asynchronous `generateQRCode`. ([\matrix-org#3562](matrix-org#3562)).
* Deprecate `VerificationRequest.beginKeyVerification()` in favour of `VerificationRequest.startVerification()`. ([\matrix-org#3528](matrix-org#3528)).
* Deprecate `Crypto.VerificationRequest` application event, replacing it with `Crypto.VerificationRequestReceived`. ([\matrix-org#3514](matrix-org#3514)).
* Throw saner error when peeking has its room pulled out from under it ([\matrix-org#3577](matrix-org#3577)). Fixes element-hq/element-web#18679.
* OIDC: Log in ([\matrix-org#3554](matrix-org#3554)). Contributed by @kerryarchibald.
* Prevent threads code from making identical simultaneous API hits ([\matrix-org#3541](matrix-org#3541)). Fixes element-hq/element-web#25395.
* Update IUnsigned type to be extensible ([\matrix-org#3547](matrix-org#3547)).
* add stop() api to BackupManager for clean shutdown ([\matrix-org#3553](matrix-org#3553)).
* Log the message ID of any undecryptable to-device messages ([\matrix-org#3543](matrix-org#3543)).
* Ignore thread relations on state events for consistency with edits ([\matrix-org#3540](matrix-org#3540)).
* OIDC: validate id token ([\matrix-org#3531](matrix-org#3531)). Contributed by @kerryarchibald.
* Fix read receipt sending behaviour around thread roots ([\matrix-org#3600](matrix-org#3600)).
* Fix `TypedEventEmitter::removeAllListeners(void)` not working ([\matrix-org#3561](matrix-org#3561)).
* Don't allow Olm unwedging rate-limiting to race ([\matrix-org#3549](matrix-org#3549)). Fixes element-hq/element-web#25716.
* Fix an instance of failed to decrypt error when an in flight `/keys/query` fails. ([\matrix-org#3486](matrix-org#3486)).
* Use the right anchor emoji for SAS verification ([\matrix-org#3534](matrix-org#3534)).
* fix a bug which caused the wrong emoji to be shown during SAS device verification. ([\matrix-org#3523](matrix-org#3523)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@t3chguy t3chguy

Labels
O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Prevent threads code from making identical simultaneous API hits matrix-org/matrix-js-sdk
7 participants
@andybalaam @clokep @Johennes @turt2live @richvdh @t3chguy @justjanne