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

Switch to reqwest #332

Merged
merged 14 commits into from
Oct 18, 2024
Merged

Switch to reqwest #332

merged 14 commits into from
Oct 18, 2024

Conversation

gferon
Copy link
Collaborator

@gferon gferon commented Oct 16, 2024

Switch the concrete implementation of both the HTTP client to reqwest and reqwest-websocket which are powered behind the scenes by exactly the same libraries we are currently using (namely, hyper and tokio-tungstenite).

  • Using a higher level builder-style API makes it easier to add support for new endpoints, and removes a lot of glue code we had to acquire over the year (the code looks now much closer to PushService.kt).
  • reqwest::Client is now shared by both HTTP requests and the Websocket
  • Remove one level of sink and stream in the websocket impl (since you don't have to forward messages to the concrete implementation)
  • Introduce a no-op trait called SignalServiceResponse which is implemented for both reqwest::Response and WebSocketResponseMessage to let us share the HTTP status error handling (the one that handles mismatched sessions, etc.) - this is IMO a big improvement and will avoid any sort of drifting in the future

@gferon gferon marked this pull request as ready for review October 17, 2024 10:41
Base automatically changed from push-service-trait-no-more to main October 17, 2024 14:46
Copy link
Contributor

@direc85 direc85 left a comment

Choose a reason for hiding this comment

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

+573 −1,050

Everybody liked that

(Except Clippy. Clippy has no chill.)

I gave this a quick read and it really seems to simmer the code down nicely!

src/push_service/cdn.rs Outdated Show resolved Hide resolved
@gferon gferon requested review from rubdos and direc85 October 17, 2024 21:45
@gferon
Copy link
Collaborator Author

gferon commented Oct 17, 2024

+573 −1,050

Everybody liked that

(Except Clippy. Clippy has no chill.)

I gave this a quick read and it really seems to simmer the code down nicely!

I have ruined my statistics a little bit by introducing a better abstraction for handling errors (which means we don't have duplicated logic in PushService and WebSocketService anymore 🎉) - however, I believe the tradeoff of a smaller statistical achievement for a higher quality library is worth it 🧌

I think this is ready for review, I will not touch this PR again before it becomes an insatiable monster. I still believe it is quite OK to test and will be running presage on top of it for the next few days (the only part that I'm worried about would be attachments, but this can easily be tested.)

@rubdos
Copy link
Member

rubdos commented Oct 17, 2024

Haven't looked into detail too much yet, but this looks very good already. I suggest we also make a WF build with this and run it some days.

@gferon gferon enabled auto-merge (squash) October 18, 2024 21:18
@gferon gferon disabled auto-merge October 18, 2024 21:19
@gferon gferon merged commit 9782968 into main Oct 18, 2024
7 checks passed
@gferon gferon deleted the reqwest branch October 18, 2024 21:20
@rubdos
Copy link
Member

rubdos commented Oct 20, 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