-
Notifications
You must be signed in to change notification settings - Fork 26
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
pubsys: switch to tough async #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the full async conversion, we'll want to look at replacing std
with tokio
in several spots:
std::io
std::fs
std::process
For example, upload-ova
uses a std-process
to run govc
inside a container. That's not going to do the executor any favors.
Fix cargo deny issues (including temporary fixes). Squash to one commit. |
Eliminate extra runtime creations and use |
Small fixups from my own comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me, nice work. I only have one concern.
let _permit = cloned_sem.acquire().await.context(error::SemaphoreSnafu)?; | ||
download_target(cloned_repo, cloned_target).await | ||
}; | ||
tasks.spawn(fut); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could spawn an unbounded number of tasks. Before reading this code, my understanding was that if we are using the multi-threaded runtime this could therefor result in an unbounded number of OS threads being spawned. After re-reading the runtime docs it seems like the number of OS threads is bound to the number of CPU cores.
In the past I've used buffered_unordered
to limit the number of tasks as well, which seems slightly safer but it is certainly more complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than Sean's comment
Attempt to use buffered_unordered. Will test. |
Restore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 🚀
Issue #, if available:
Description of changes:
Use the new async version of tough and tuftool
Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.