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

Resolve unwrap in build body and when acquiring RwLock #79

Merged

Conversation

threema-donat
Copy link
Contributor

@threema-donat threema-donat commented Apr 24, 2024

Description

The current version does panic if a RwLock is poisoned and also when a HTTP request could not be constructed from the given user data.

This PR replaces the RwLocks with the ones from parking_lot that do not have poison errors and adds propagation of errors happening when building the http request.

Resolves #69

How Has This Been Tested?

A test has been written for the invalid payload.

Due Dilligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@chris13524
Copy link
Member

Curious: why not use tokio's RwLock which also does not return Result? Looks like some functions are not currently async but perhaps they could be made such.

CHANGELOG.md Outdated
@@ -1,5 +1,9 @@
# Changelog

## Unreleased

- fix: Avoid panics when acquiring a RwLock<_> and when building HTTP requests
Copy link
Member

Choose a reason for hiding this comment

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

Looks like breaking change; we should bump to 0.7.0. I will do after merging

@threema-donat
Copy link
Contributor Author

Curious: why not use tokio's RwLock which also does not return Result? Looks like some functions are not currently async but perhaps they could be made such.

The maintainers of tokio itself say that one should not use the async version of Mutex / RwLock without having reason for that: link. The same can very likely also be said about RwLock.

For this reason, I think it's better to use parking_lot.

@chris13524
Copy link
Member

Please resolve conflicts :)

Copy link
Member

@chris13524 chris13524 left a comment

Choose a reason for hiding this comment

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

Thanks!

@chris13524 chris13524 merged commit ed825b4 into WalletConnect:master Apr 26, 2024
5 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.

Sending a payload with invalid uri chars panics
2 participants