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

Element-R: Emit CryptoEvent.UserTrustStatusChanged when user identity is updated #3716

Merged
merged 4 commits into from
Sep 8, 2023

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Sep 7, 2023

Fixes https://github.com/vector-im/crypto-internal/issues/148


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

@richvdh richvdh added the T-Task Tasks for the team like planning label Sep 7, 2023
@richvdh richvdh changed the title Emit CryptoEvent.UserTrustStatusChanged when user identity is updated Element-R: Emit CryptoEvent.UserTrustStatusChanged when user identity is updated Sep 7, 2023
@richvdh
Copy link
Member Author

richvdh commented Sep 7, 2023

This is currently awaiting a release of rust-sdk-crypto-wasm, but more difficult than that: it introduces a reference cycle in the rust side.

Basically, once I call OlmMachine::user_identities_stream, the cryptostore isn't being dropped when the OlmMachine is dropped.

What seems to be happening is as follows:

  • user_identities_stream takes a copy of the VerificationMachine. The new copy holds a ref to the CryptoStoreWrapper.
  • The new copy is used in a Stream::map closure ... so is retained until the stream is dropped.
  • I have a reader which reads the stream to exhaustion
  • The stream reaches exhaustion when the sender end of the BroadcastStream is dropped
  • However, the sender is a member of CryptoStoreWrapper, so is never dropped, because of the reference in the Stream::map closure.

So in short, we have a reference cycle which stops the store being cleaned up.

@richvdh
Copy link
Member Author

richvdh commented Sep 7, 2023

but more difficult than that: it introduces a reference cycle in the rust side.

This is fixed by matrix-org/matrix-rust-sdk-crypto-wasm#31

Copy link
Contributor

@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 good, as far as I understand it, which is not a great deal.

@richvdh richvdh added this pull request to the merge queue Sep 8, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 8, 2023
@andybalaam
Copy link
Contributor

I suspect the test failure was flakiness, re-adding to merge queue.

For the record, the failure was:

FAIL test/components/views/settings/JoinRuleSettings-test.tsx (7.445 s)
  ● <JoinRuleSettings /> › knock rooms › when room does not support join rule knock › upgrades room when changing join rule to knock

    expect(element).toBeInTheDocument()

    element could not be found in the document

      227 |
      228 |                 // update spaces
    > 229 |                 expect(await screen.findByText("Updating space...")).toBeInTheDocument();
          |                                                                      ^
      230 |
      231 |                 await flushPromises();
      232 |

      at Object.toBeInTheDocument (test/components/views/settings/JoinRuleSettings-test.tsx:229:70)

@andybalaam andybalaam added this pull request to the merge queue Sep 8, 2023
@andybalaam
Copy link
Contributor

If it merges, we should add a flaky test issue, similar but not identical to element-hq/element-web#25625

Merged via the queue into develop with commit f963ca5 Sep 8, 2023
20 checks passed
@andybalaam andybalaam deleted the rav/element-r/user_trust_status_changed branch September 8, 2023 04:52
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.

2 participants