matrix-js-sdk icon indicating copy to clipboard operation
matrix-js-sdk copied to clipboard

Making IgnoredInvites mostly sync.

Open Yoric opened this issue 1 year ago • 4 comments

Experience and feedback from https://github.com/matrix-org/matrix-react-sdk/pull/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

  • [X] Tests written for new code (and old code if feasible)
  • [X] Linter and other CI checks pass
  • [X] 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

Yoric avatar Sep 08 '22 08:09 Yoric

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

Yoric avatar Sep 23 '22 12:09 Yoric

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

t3chguy avatar Sep 23 '22 12:09 t3chguy

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 https://github.com/matrix-org/matrix-spec-proposals/pull/3847, iirc.

Yoric avatar Oct 21 '22 14:10 Yoric

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.

Yoric avatar Nov 03 '22 12:11 Yoric