pinned events: limit the max amount of concurrent requests to load pinned events #3878

jmartinesp · 2024-08-22T11:18:08Z

Depends on #3840.

RequestConfig::max_concurrent_requests won't work for requests to a same endpoint, since it's a global value that can only be passed to HttpClient when created.

To replace it I added a buffered stream that can run at most MAX_PINNED_EVENTS_CONCURRENT_REQUESTS (10) requests concurrently.

There is also a test to make sure this max concurrency mechanism works.

Public API changes documented in changelogs (optional)

Signed-off-by:

andybalaam

Looks good, one question.

andybalaam · 2024-08-22T12:16:02Z

crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs

+    // Give it time to load events. As each request takes `request_delay`, we should
+    // have exactly `MAX_PINNED_EVENTS_CONCURRENT_REQUESTS` if the max
+    // concurrency setting is honoured.
+    sleep(Duration::from_millis(request_delay)).await;


Would it be safer to sleep for 1.5 * request_delay just to make sure some other delay doesn't cause us to just miss it?

If we do that I'm pretty sure we'll get 20 instead since the 2nd batch of requests will be running by then.

EDIT: yep, we do.

Verifications failed: - Mock #0. Expected range of matching incoming requests: == 10 Number of matched incoming requests: 20

Then 0.5 * request_delay?

Ok, sounds good. I wanted to use the full delay to prove this worked as expected, but it's true it might result in flaky tests 🫤 .

Hywan · 2024-08-23T07:51:08Z

crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs

@@ -26,7 +26,7 @@ use tracing::{debug, warn};

 use crate::timeline::event_handler::TimelineEventKind;

-const MAX_CONCURRENT_REQUESTS: usize = 10;
+pub const MAX_PINNED_EVENTS_CONCURRENT_REQUESTS: usize = 10;


If it's a pub item, please add a documentation to it.

If it's a pub only for testing purposes, maybe adding a #[cfg(test)] would be nice?

If it's a pub item, please add a documentation to it.

True, my bad.

If it's a pub only for testing purposes, maybe adding a #[cfg(test)] would be nice?

Well, it just needs to be pub for the tests, but it needs to exist in general since it's used in the code below it. I don't know if there's some way to do that or it's better to just keep it public.

That's one limitation of Rust where we cannot add a #[cfg] for a visibility only. There is a crate to handle that, https://crates.io/crates/cfg-vis, but we have removed it recently.

You can write this as follows:

#[cfg(test)] pub const … = …; #[cfg(not(test))] const … = …;

if you don't want to repeat the constant value, you can have a private constant just for that:

const VALUE: usize = 10; #[cfg(test)] pub const …: usize = VALUE; #[cfg(not(test))] const …: usize = VALUE;

of course, with better names that … and VALUE :-D.

It seems like this won't work as the target test is an integration one.

Can I have another review, pretty please 🙏 ?

From the Rust docs:

In Rust, integration tests are entirely external to your library. They use your library in the same way any other code would, which means they can only call functions that are part of your library’s public API. Their purpose is to test whether many parts of your library work together correctly. Units of code that work correctly on their own could have problems when integrated, so test coverage of the integrated code is important as well. To create integration tests, you first need a tests directory.

So I guess the test and testing cfgs are not used and we're back to "It seems like this won't work as the target test is an integration one." 🫤 .

Since this apparaently can't be done and the doc comment was added as @Hywan requested, can we have another review so we can at least make some progress?

I've found a better idea, but it involves a bit more work (sowwy). Since the PinnedEventLoader can use an arbitrary PinnedEventsRoom impl, can you make the test a non-integration test, at the bottom of this file, using a test impl for PinnedEventsRoom? Maybe running the assertions will be even simpler with that, as we don't have to rely on wiremock anymore?

Honestly, wiremock is quite the help here... I'd rather not change everything for just a pub modifier. Maybe I can add this to be part of the TimelineFocus::PinnedEvents instead, like max_events_to_load?

It would also make the testing more targeted by not involved network, and make it much faster to run, but I understand this is a big ask.

Maybe I can add this to be part of the TimelineFocus::PinnedEvents instead, like max_events_to_load?

Yep, sounds great, thanks!

codecov · 2024-08-23T13:29:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.18%. Comparing base (1eecb2d) to head (742b987).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3878      +/-   ##
==========================================
+ Coverage   84.17%   84.18%   +0.01%     
==========================================
  Files         267      267              
  Lines       28181    28181              
==========================================
+ Hits        23720    23725       +5     
+ Misses       4461     4456       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr · 2024-08-26T08:34:10Z

crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs

+        let request_config = Some(RequestConfig::default().retry_limit(3));
+
+        let mut loaded_events: Vec<SyncTimelineEvent> =
+            stream::iter(pinned_event_ids.into_iter().map(|event_id| {


Wonder if we could use a plain Semaphore instead of a stream here, it's a bit overkill to use the stream machinery for this IMO…

We could and did, but the code is more readable like this (IMO) and there's no performance penalty as far as I could tell by the results of the benchmark. Also, it's used in a very similar way in some other parts of the SDK.

If we wanted to use a Semaphore instead, what would the semaphore.acquire() code look like if the result contains an error? I remember we discussed it in a previous PR but I'm not sure if we ever made a decision.

the code is more readable like this (IMO)

I find the code much worse, because it involves other async concepts that one must be aware of (streams), and higher-level typing that's hard to grasp just from reading the code (stream flattening). But let's keep it like this now, and we can change that later, if it's unbearable :)

jmartinesp · 2024-09-05T08:01:55Z

This has been with 'changes requested' for > 2 weeks, but the requested changes can't be done (agreed on that in a chat room). Hywan is not working today either, so can I just rebase and force merge it?

bnjbvr · 2024-09-05T08:07:54Z

No, PRs need to be approved before getting merged.

jmartinesp · 2024-09-05T08:15:39Z

Then we're lucky we're not in a rush, I guess.

bnjbvr

Thanks, approving assuming my comments will be addressed.

bnjbvr · 2024-09-05T14:43:03Z

crates/matrix-sdk-ui/src/timeline/pinned_events_loader.rs

-                        warn!("error when loading pinned event: {err}");
-                        None
+        let request_config = Some(RequestConfig::default().retry_limit(3));
+        let max_concurrent_requests = self.max_concurrent_requests;


Can you inline this variable, as it's only used once?

bnjbvr · 2024-09-05T14:43:40Z

crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs

+        .with_focus(TimelineFocus::PinnedEvents {
+            max_events_to_load: 100,
+            max_concurrent_requests: 10,
+        })


Can you make a tiny test helper that creates a pinned events focused timeline, instead? that will avoid repeating the new argument everywhere

bnjbvr · 2024-09-05T14:44:43Z

crates/matrix-sdk-ui/tests/integration/timeline/pinned_event.rs

+        )));
+
+    // Amount of time to delay the response of an /event mock request, in ms.
+    let request_delay = 50;


Is 50 milliseconds enough for the code cov task? That would be fine to use something greater up to 1 second, fwiw.

From the codecov report it seems like those lines are counted as covered too, so maybe there's no need to increase this.

jmartinesp requested a review from a team as a code owner August 22, 2024 11:18

jmartinesp requested review from andybalaam and removed request for a team August 22, 2024 11:18

andybalaam approved these changes Aug 22, 2024

View reviewed changes

jmartinesp force-pushed the jme/update-pinned-events branch from c86c842 to b8f24bd Compare August 22, 2024 12:51

jmartinesp force-pushed the fix/jme/max-concurrency-in-pinned-events-timeline branch from 2e22282 to fdb0b01 Compare August 22, 2024 14:06

Hywan requested changes Aug 23, 2024

View reviewed changes

jmartinesp force-pushed the jme/update-pinned-events branch 2 times, most recently from 4f57f54 to 7cfcbae Compare August 23, 2024 11:58

Base automatically changed from jme/update-pinned-events to main August 23, 2024 12:12

An error occurred while trying to automatically change base from jme/update-pinned-events to main August 23, 2024 12:12

jmartinesp force-pushed the fix/jme/max-concurrency-in-pinned-events-timeline branch from 9d6c03a to 08ba529 Compare August 23, 2024 13:13

jmartinesp requested a review from Hywan August 23, 2024 13:16

bnjbvr reviewed Aug 26, 2024

View reviewed changes

bnjbvr changed the title ~~fix(pinned_events): limit the max amount of concurrent requests to load pinned events~~ pinned events: limit the max amount of concurrent requests to load pinned events Sep 3, 2024

jmartinesp force-pushed the fix/jme/max-concurrency-in-pinned-events-timeline branch from 5439303 to 9c25610 Compare September 3, 2024 11:56

bnjbvr approved these changes Sep 5, 2024

View reviewed changes

jmartinesp force-pushed the fix/jme/max-concurrency-in-pinned-events-timeline branch from 5e9cf98 to 0f8a821 Compare September 9, 2024 07:10

Hywan removed their request for review September 9, 2024 07:21

Hywan approved these changes Sep 9, 2024

View reviewed changes

sdk-ui: fix max concurrent requests for pinned events timeline.

742b987

jmartinesp force-pushed the fix/jme/max-concurrency-in-pinned-events-timeline branch from 0f8a821 to 742b987 Compare September 9, 2024 07:24

jmartinesp enabled auto-merge (rebase) September 9, 2024 07:27

jmartinesp merged commit 10a0d59 into main Sep 9, 2024
40 checks passed

jmartinesp deleted the fix/jme/max-concurrency-in-pinned-events-timeline branch September 9, 2024 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pinned events: limit the max amount of concurrent requests to load pinned events #3878

pinned events: limit the max amount of concurrent requests to load pinned events #3878

jmartinesp commented Aug 22, 2024

andybalaam left a comment

andybalaam Aug 22, 2024

jmartinesp Aug 22, 2024 •

edited

Loading

andybalaam Aug 22, 2024

jmartinesp Aug 22, 2024

Hywan Aug 23, 2024

jmartinesp Aug 23, 2024

Hywan Aug 23, 2024 •

edited

Loading

jmartinesp Aug 23, 2024

jmartinesp Sep 2, 2024

jmartinesp Sep 5, 2024 •

edited

Loading

jmartinesp Sep 5, 2024

bnjbvr Sep 5, 2024

jmartinesp Sep 5, 2024

bnjbvr Sep 5, 2024

codecov bot commented Aug 23, 2024 •

edited

Loading

bnjbvr Aug 26, 2024

jmartinesp Sep 2, 2024 •

edited

Loading

bnjbvr Sep 2, 2024 •

edited

Loading

jmartinesp commented Sep 5, 2024

bnjbvr commented Sep 5, 2024

jmartinesp commented Sep 5, 2024

bnjbvr left a comment

bnjbvr Sep 5, 2024

bnjbvr Sep 5, 2024

bnjbvr Sep 5, 2024

jmartinesp Sep 6, 2024

pinned events: limit the max amount of concurrent requests to load pinned events #3878

pinned events: limit the max amount of concurrent requests to load pinned events #3878

Conversation

jmartinesp commented Aug 22, 2024

andybalaam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hywan Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jmartinesp Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

bnjbvr Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

jmartinesp commented Sep 5, 2024

bnjbvr commented Sep 5, 2024

jmartinesp commented Sep 5, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Aug 22, 2024 •

edited

Loading

Hywan Aug 23, 2024 •

edited

Loading

jmartinesp Sep 5, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

jmartinesp Sep 2, 2024 •

edited

Loading

bnjbvr Sep 2, 2024 •

edited

Loading