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

client builder: return a ClientBuildError when failing to build, instead of filtering out unexpected errors #4016

Merged

Conversation

reivilibre
Copy link
Contributor

@reivilibre reivilibre commented Sep 17, 2024

This old method was checking invariants that were
spooky-action-at-a-distance: these invariants have changed since then,
so this would panic instead of returning a proper error to the caller.

Signed-off-by: [email protected]

@reivilibre reivilibre requested a review from a team as a code owner September 17, 2024 18:34
@reivilibre reivilibre requested review from bnjbvr and removed request for a team September 17, 2024 18:34
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

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

Project coverage is 84.26%. Comparing base (8119697) to head (2a3111a).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/client/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4016      +/-   ##
==========================================
+ Coverage   84.25%   84.26%   +0.01%     
==========================================
  Files         266      266              
  Lines       28344    28341       -3     
==========================================
+ Hits        23880    23882       +2     
+ Misses       4464     4459       -5     

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

bnjbvr
bnjbvr previously requested changes Sep 18, 2024
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It's bad that you can hit this panic, I suppose it was put here as an internal-API error, and never supposed to be triggerable by users of the SDK. I think the code has evolved sufficiently that it's a bad idea for us to use assert_valid_builder_args at all: the error could also be a store error, or a discovery error, or, or, or…

So instead, I think we should:

  • get rid of assert_valid_builder_args
  • have Client::new return a Result<Self, ClientBuildError>.

Are you willing to make the changes here? Otherwise one of us can probably get to it at some point.

@reivilibre
Copy link
Contributor Author

I can have a go at that soon, will have to happen after the mad rush to get this other stuff done that led to this issue.

BTW it was a SqliteStore(MissingVersion).... 🤷

This old method was checking invariants that were
spooky-action-at-a-distance: these invariants have changed since then,
so this would panic instead of returning a proper error to the caller.
@bnjbvr bnjbvr changed the title Make unreachable! ClientBuildError assertion give a descriptive error client builder: return a ClientBuildError when failing to build, instead of filtering out unexpected errors Sep 30, 2024
@bnjbvr bnjbvr requested review from a team, Hywan, bnjbvr and stefanceriu and removed request for a team, Hywan and bnjbvr September 30, 2024 10:13
@bnjbvr bnjbvr dismissed their stale review September 30, 2024 10:14

i pushed onto the branch

Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

@bnjbvr bnjbvr merged commit 866b6e5 into matrix-org:main Sep 30, 2024
40 checks passed
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