-
Notifications
You must be signed in to change notification settings - Fork 38
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
Adds optional async interface #126
Conversation
Cool! I didn’t know async trait was a thing yet. Would you mind splitting this PR in 2? So we can easily review first the code cleanup, then the async one? |
For sure! Sounds like a good idea. I'll pull the refactoring out into a separate PR. Will also add some tests for the async stuff in here. |
Also, this would need to PR against the |
Ha! I did not even know there was a dev branch (used to PRing against |
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.
A couple of interesting things to consider before the next version of this.
/// Main function of the library. | ||
/// Finds a set of packages satisfying dependency bounds for a given package + version pair. | ||
#[cfg(feature = "async")] | ||
pub async fn resolve_async<P: Package, V: Version>( |
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.
It seems unfortunate for us to need to completely duplicate all of this code.
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.
I definitely agree.
My rough idea is to refactor this method into smaller methods, such that there is no duplication, only the async parts need to be duplicated.
I will wait for the refactoring PR (#127) to go through before starting this however.
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.
Taking long methods and splitting them up seems like good value all on its own. I look forward to seeing your work on it.
.should_cancel() | ||
.map_err(|err| PubGrubError::ErrorInShouldCancel(err))?; | ||
|
||
state.unit_propagation(next)?; |
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 function call is usually cheap, but sometimes it will need to do a lot of work (potentially exponential amount of work) and will therefore block the asynchronous runner. On the other hand, if we do some kind of spawn_blocking
call here the overhead will be significant as the work is almost always trivial.
I think this is a profound and serious issue we are going to need to explore before we can merge something like this.
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.
I was very lucky to get @carllerche's opinion on this. The two alternatives he suggested were:
- Have
unit_propagation
to a fixed amount of work and if there's more work to be done do the rest in aspon_blocking
. In theory, this is the best solution. In practice I'm not sure that we can clearly identify how much work is being done by each sub call withinunit_propagation
, so I don't know if it's practical. We would also need extensive tests to verify that we don't accidentally have a loop occurring before we callspon_blocking
. - Do not provide an async interface. Callers can
block_in_place
before calling resolve, and calltokio::runtime::Handle::block_on
within the callbacks.
I'm not sure which solution is more unpleasant/inefficient, but it's comforting to know that it is a real problem.
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.
To be honest, I did not know of the block_in_place
approach, but that sounds like something that might be easy to use and still provide the ability to interface with async code.
Maybe one idea is to add this to the documentation with some code examples? This should be a good starting point for other people that interface with async stuff.
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.
My main concern with that approach is how it works with other executors. It can work well for Tokyo, but are there equivalents in the others? What about when running as Wasm in a browser?
Anyway, 110% on documentation. We should have a section in the book about how to shoehorn asynchronous communication into the current API. That, to the best of our ability, documents the available options and their advantages and disadvantages. That unblocks people without waiting for us to release a new version of the crate.
}; | ||
|
||
let decision = dependency_provider | ||
.choose_package_version(potential_packages) |
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.
It would be really nice if the asynchronous calls to get package information can be run in parallel allowing this await to return when the first of them returns. Of course this would make reproducibility much more complicated, and would need to be documented carefully. But it should make many real-world cases much more efficient.
Hey!
In reference to #110, this adds an optional
async
feature which, when enabled, enables theAsyncDependencyProvider
trait.This PR also cleans up the
resolver
code somewhat (I like it when code is not nested too deeply, as it makes it easier for me to follow).