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

Split client to async and blocking #396

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

2e3s
Copy link
Contributor

@2e3s 2e3s commented Jun 18, 2023

The problem is not only that the current functionality is less useful in async context. The client cannot be used in async context at all as is:
https://docs.rs/reqwest/latest/reqwest/blocking/

Conversely, the functionality in reqwest::blocking must not be executed within an async runtime, or it will panic when attempting to block

I've used tokio's runtime to block because reqwest uses tokio blocking internally as well.
https://github.com/seanmonstar/reqwest/blob/master/src/blocking/wait.rs
It also uses a current thread runtime, but with timeouts. However, the async client specified the timeout already, so it shouldn't matter much.

@2e3s
Copy link
Contributor Author

2e3s commented Jun 18, 2023

This watcher doesn't use rev to depend on a commit or any pin of the kind https://github.com/ActivityWatch/aw-watcher-window-wayland/blob/master/Cargo.toml (which is not good exactly because of this), so it will have to be updated to be built successfully, if this change is accepted.

@2e3s
Copy link
Contributor Author

2e3s commented Jun 28, 2023

One alternative would be "async generic" so to speak: https://docs.rs/maybe-async/0.2.6/maybe_async/ - it would exclude tokio from crate's dependencies (reqwest would use it regardless), maybe increase build time, remove some code duplication.

@ErikBjare
Copy link
Member

Hmm, can't figure out why CI doesn't run

Reqwest uses tokio blocking internally as well.
@ErikBjare
Copy link
Member

CI passed fine in #419, force-pushed the rebased commit here and merging :)

@ErikBjare ErikBjare merged commit e689636 into ActivityWatch:master Sep 12, 2023
12 of 14 checks passed
@2e3s 2e3s deleted the async-client branch September 13, 2023 11:15
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.

2 participants