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

crypto: Expose new stream about room_key withheld messages #3660

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jul 5, 2024

Part of the fix to element-hq/element-web#27653.

[Please ignore the branch name; I appear to have fat-fingered]

@richvdh richvdh requested a review from a team as a code owner July 5, 2024 17:25
@richvdh richvdh requested review from Hywan and removed request for a team July 5, 2024 17:25
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.30%. Comparing base (fac7221) to head (6b38e99).
Report is 18 commits behind head on main.

Files Patch % Lines
...atrix-sdk-crypto/src/store/crypto_store_wrapper.rs 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3660      +/-   ##
==========================================
+ Coverage   84.25%   84.30%   +0.04%     
==========================================
  Files         259      259              
  Lines       26622    26625       +3     
==========================================
+ Hits        22431    22446      +15     
+ Misses       4191     4179      -12     

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

Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only review the code, not the feature. A second pair of eyes from @poljar might be useful.

crates/matrix-sdk-crypto/src/machine.rs Outdated Show resolved Hide resolved
// We should receive a notification on the room_keys_withheld_received_stream
let withheld_received = room_keys_withheld_received_stream
.next()
.now_or_never()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To use now_or_never, you must be sure that the value will be immediately available in the Stream, otherwise the test will be flaky.

Is it a problem to use next().await here instead? The risk is that the test might be pending forever if something changes in the future, but it's still an indication that a regression has been detected. The pros, however, is that we are sure the test can never be flaky.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a problem to use next().await here instead?

I guess not. TBH I pretty much cargo-culted this from test_megolm_encryption above, which afaik has never shown any sign of being flaky. Flipping it around: is there any reason to believe that the value would not already be available in the Stream?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stream is created before we receive the sync changes, the sync changes trigger an update, this means that the now_or_never() should never fail here, unless we introduce a bug.

There are no hidden tasks/threads or requests being sent out in receive_sync_changes() it's all a pretty dumb, for better or worse, state machine.

now_or_never() is the right thing to use, otherwise a regression will produce a hang instead of a loud test failure.

crates/matrix-sdk-crypto/src/store/crypto_store_wrapper.rs Outdated Show resolved Hide resolved
@Hywan Hywan requested a review from poljar July 8, 2024 12:00
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this looks fine. I would probably add a doc example but not required since it's a simple stream and we have plenty examples for streams.

// We should receive a notification on the room_keys_withheld_received_stream
let withheld_received = room_keys_withheld_received_stream
.next()
.now_or_never()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stream is created before we receive the sync changes, the sync changes trigger an update, this means that the now_or_never() should never fail here, unless we introduce a bug.

There are no hidden tasks/threads or requests being sent out in receive_sync_changes() it's all a pretty dumb, for better or worse, state machine.

now_or_never() is the right thing to use, otherwise a regression will produce a hang instead of a loud test failure.

@poljar
Copy link
Contributor

poljar commented Jul 9, 2024

Oh and thanks for taking the time to refactor the filter we had for each stream.

@richvdh richvdh enabled auto-merge (squash) July 9, 2024 14:02
@richvdh richvdh force-pushed the rav/indexeddb_storage_efficiency_fix_bugs branch from 8460281 to b74cf0a Compare July 9, 2024 14:03
@richvdh richvdh force-pushed the rav/indexeddb_storage_efficiency_fix_bugs branch from b74cf0a to 6b38e99 Compare July 9, 2024 14:09
@richvdh richvdh merged commit 2d3e2da into main Jul 9, 2024
40 checks passed
@richvdh richvdh deleted the rav/indexeddb_storage_efficiency_fix_bugs branch July 9, 2024 17:55
richvdh added a commit to matrix-org/matrix-rust-sdk-crypto-wasm that referenced this pull request Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants