Making IgnoredInvites mostly sync. #2653

Yoric · 2022-09-08T08:38:53Z

Experience and feedback from matrix-org/matrix-react-sdk#9255 indicates that we cannot integrate an async IgnoredInvites.getRuleForInvite. This PR:

rewrites getRuleForInvite to be sync;
adds some locking within IgnoredInvites to avoid race conditions noticed by @t3chguy.

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

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

Type: task

src/models/invites-ignorer.ts

Yoric · 2022-09-23T12:57:08Z

@t3chguy Is this any better? The any code makes it clearer what data has been validated and what data hasn't.

t3chguy · 2022-09-23T12:58:56Z

@Yoric sorry going to have to bump this back to the queue due to other obligations

turt2live

sorry, I'm coming at this blind with no real idea what troubles you're facing on the react-sdk side: it may be worth a visit to Element Web Internal to talk about those challenges instead, as making things synchronous can be annoyingly more difficult than understanding the async code path.

This is also much harder to review because there's no MSC reference within the area of the diff - it's hard to see what the underlying data structures are without such a reference.

src/models/invites-ignorer.ts

turt2live · 2022-09-23T22:26:53Z

src/models/invites-ignorer.ts

+            this.getOrCreateTargetRoomPromise = (async () => {
+                // We need to create our own policy room for ignoring invites.
+                target = (await this.client.createRoom({
+                    name: "Individual Policy Room",


Should this be translated?

Oh, good point. Is there any way to do that in matrix-js-sdk? Or should I somehow pass that string from react-sdk?

turt2live · 2022-09-23T22:27:44Z

src/models/invites-ignorer.ts

+        try {
+            // Nobody is calling the method at the moment.
+            // Register ourselves as the leader and start creating our policy room.
+            this.getOrCreateTargetRoomPromise = (async () => {


typically our pattern is = new Promise((resolve, reject) => { ... }); instead

With two calls to then after the Promise? Or is it an async (resolve, reject) => { ... }?

I've tried rewriting this call to use the Promise constructor but it didn't really feel readable.

turt2live · 2022-09-23T22:28:09Z

src/models/invites-ignorer.ts

     */
-    private getIgnoreInvitesPolicies(): {[key: string]: any} {
+    private getIgnoreInvitesPolicies(): any {


this needs a type - what is it?

this needs a type - what is it?

It's data coming from Matrix, therefore untrusted. Until I have checked it (which is what the caller functions do), I have no clue. The any is there specifically to mark that it's untrusted. Is there a better way to write it down?

turt2live · 2022-09-23T22:28:21Z

src/models/invites-ignorer.ts

     */
    private getPoliciesAndIgnoreInvitesPolicies():
-        {policies: {[key: string]: any}, ignoreInvitesPolicies: {[key: string]: any}} {
+        {policies: any, ignoreInvitesPolicies: any} {


these also need types

Same response, sadly. If there is a better way to mark untrusted-deserialized-json than any, I'll be happy to use it!

src/models/invites-ignorer.ts

turt2live · 2022-09-23T22:34:34Z

src/models/invites-ignorer.ts

+        // Thanks to run-to-completion, the uninterruptible behavior of this
+        // method is the following:


This comment (and the lines which follow) aren't really describing anything: it's just saying what we're about to do, not why it's important to have a lock. We know what it's about to do: there's code to tell us :)

We should also describe why we don't need cross-client locking, considering this code is seemingly important.

It's further a bad smell that we need a lock at all - why would this code be called multiple times?

This comment (and the lines which follow) aren't really describing anything: it's just saying what we're about to do, not why it's important to have a lock. We know what it's about to do: there's code to tell us :)

...because of race conditions? I'm reading, modifying, writing some user account data, that's inherently racy.

We should also describe why we don't need cross-client locking, considering this code is seemingly important.

Wait, we have cross-client locking? Where do I find the documentation on these?

It's further a bad smell that we need a lock at all - why would this code be called multiple times?

I come from a safety background. Any code is considered to be called multiple times unless proven otherwise :)

Yoric · 2022-10-21T14:06:37Z

sorry, I'm coming at this blind with no real idea what troubles you're facing on the react-sdk side: it may be worth a visit to Element Web Internal to talk about those challenges instead, as making things synchronous can be annoyingly more difficult than understanding the async code path.

Well, I've been asked by my react-sdk reviewer to make things sync :)

This is also much harder to review because there's no MSC reference within the area of the diff - it's hard to see what the underlying data structures are without such a reference.

Should be matrix-org/matrix-spec-proposals#3847, iirc.

turt2live

this appears to have failing checks, and it's still not really clear why this PR needs to exist. Usually we're heavily in support of async code, so I'm surprised we want to make it sync.

It's also hard to re-review this at the moment: all the conversations have been left unresolved, making it unclear which need further comment.

Yoric · 2022-11-03T12:09:56Z

this appears to have failing checks, and it's still not really clear why this PR needs to exist. Usually we're heavily in support of async code, so I'm surprised we want to make it sync.

It's also hard to re-review this at the moment: all the conversations have been left unresolved, making it unclear which need further comment.

To clarify:

This PR exists because @t3chguy requested that I make the code sync here. Should I change the commit message to clarify this?
I re-requested review because at least 3 conversations are waiting for a response on your part. I realize that I should have cleared up the resolved conversations, my bad.
Regarding the test failures, if someone could give me a hand understanding them, I would be grateful. All these test failures show up in code that, as far as I can tell, I haven't impacted, even from afar.

@t3chguy

Experience and feedback from matrix-org/matrix-react-sdk#9255 indicates that we cannot integrate an async `IgnoredInvites.getRuleForInvite`. This PR: - rewrites `getRuleForInvite` to be sync; - adds some locking within `IgnoredInvites` to avoid race conditions noticed by @t3chguy.

florianduros · 2024-10-31T10:09:51Z

Hi!

The PR is 2 years old and stuck.
I'm closing it. Feel free to reopen it if you want to continue to work on it.

Yoric requested a review from a team as a code owner September 8, 2022 08:38

Yoric requested review from t3chguy, duxovni and SimonBrandner September 8, 2022 08:38

github-actions bot added the T-Task Tasks for the team like planning label Sep 8, 2022

t3chguy reviewed Sep 8, 2022

View reviewed changes

src/models/invites-ignorer.ts Outdated Show resolved Hide resolved

Yoric requested a review from t3chguy September 8, 2022 15:36

Yoric mentioned this pull request Sep 8, 2022

Hide ignored invites matrix-org/matrix-react-sdk#9255

Closed

3 tasks

SimonBrandner removed request for duxovni and SimonBrandner September 9, 2022 16:42

t3chguy reviewed Sep 15, 2022

View reviewed changes

src/models/invites-ignorer.ts Outdated Show resolved Hide resolved

t3chguy reviewed Sep 15, 2022

View reviewed changes

src/models/invites-ignorer.ts Outdated Show resolved Hide resolved

src/models/invites-ignorer.ts Outdated Show resolved Hide resolved

src/models/invites-ignorer.ts Outdated Show resolved Hide resolved

src/models/invites-ignorer.ts Outdated Show resolved Hide resolved

Yoric force-pushed the yoric/ignore-invites-3847-2 branch from ff23480 to b8bb5d2 Compare September 15, 2022 15:53

Yoric requested a review from t3chguy September 23, 2022 12:57

t3chguy requested review from a team, germain-gg and turt2live and removed request for t3chguy and a team September 23, 2022 12:59

turt2live requested changes Sep 23, 2022

View reviewed changes

germain-gg removed their request for review September 30, 2022 08:20

Yoric requested review from t3chguy and turt2live October 31, 2022 08:36

turt2live requested changes Nov 3, 2022

View reviewed changes

Yoric added 2 commits November 3, 2022 13:35

WIP: Applied feedback

2c19c8d

Yoric added 4 commits November 3, 2022 13:35

WIP: Applying feedback

66ab568

WIP

5bdd718

WIP

04b284c

WIP: Applying feedback

46de018

Yoric force-pushed the yoric/ignore-invites-3847-2 branch from 56dc713 to 46de018 Compare November 3, 2022 12:37

t3chguy removed their request for review April 27, 2023 10:12

florianduros closed this Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making IgnoredInvites mostly sync. #2653

Making IgnoredInvites mostly sync. #2653

Yoric commented Sep 8, 2022 •

edited by github-actions bot

Loading

Yoric commented Sep 23, 2022

t3chguy commented Sep 23, 2022

turt2live left a comment

turt2live Sep 23, 2022

Yoric Oct 21, 2022

turt2live Sep 23, 2022

Yoric Oct 31, 2022

turt2live Sep 23, 2022

Yoric Oct 21, 2022

turt2live Sep 23, 2022

Yoric Oct 31, 2022

turt2live Sep 23, 2022

turt2live Sep 23, 2022

Yoric Oct 21, 2022

Yoric commented Oct 21, 2022

turt2live left a comment

Yoric commented Nov 3, 2022 •

edited

Loading

florianduros commented Oct 31, 2024

		// Thanks to run-to-completion, the uninterruptible behavior of this
		// method is the following:

Making IgnoredInvites mostly sync. #2653

Making IgnoredInvites mostly sync. #2653

Conversation

Yoric commented Sep 8, 2022 • edited by github-actions bot Loading

Checklist

Yoric commented Sep 23, 2022

t3chguy commented Sep 23, 2022

turt2live 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yoric commented Oct 21, 2022

turt2live left a comment

Choose a reason for hiding this comment

Yoric commented Nov 3, 2022 • edited Loading

florianduros commented Oct 31, 2024

Yoric commented Sep 8, 2022 •

edited by github-actions bot

Loading

Yoric commented Nov 3, 2022 •

edited

Loading