Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
coop: expose an unconstrained() opt-out #3547
coop: expose an unconstrained() opt-out #3547
Changes from 2 commits
a921c88
788056b
aa2cb7b
f75a467
36c6234
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should this only be available under
cfg_coop!
@carllerche ?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.
Wouldn't that mean that if you need to use this in a library you'd force your users to enable coop? (granted, I'm not sure familiar with features in Cargo, as I mentioned in that other thread we don't really use Cargo at $work :) )
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.
Ah, I remember now.
cfg_coop!
is actually a short-hand for "if any of these features are enabled". It's not a feature in and of itself. The idea is that if you don't bring in a runtime or any resource types, then there's no reason to build the coop stuff. So I thinkcfg_coop!
should also apply to these types.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.
That's a relatively complicated feature list. I might just prefer always exposing it, having it be a no-op without coop in the runtime.
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 think this was at the request of @carllerche, though his opinions may have changed since?
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 gut is that this should live in the task module for the public API. I like what @Darksonn suggests, expose the fn anytime the task module is enabled but the impl changes based on cfg_coop. Either it does something or it is a noop.
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.
Should I be making this update in this PR then? I'm not certain exactly what y'all would like me to change to get this moving forward :)
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.
Yes, please go ahead and make this change.
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.
Sounds good; thanks; I just updated the PR.
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.
Since we moved the function to
tokio::task
, this shouldn't be made public.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 kept it public so we could have access to some documentation for the coop module. Otherwise, there isn't much to explain what the wrapper does, and documenting the coop mechanism in the wrapper to disable it seems a bit counter-intuitive.
Thoughts?
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.
Hm, good point. I think I would still prefer to put it on the module docs of an existing module.
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 don't mind — where should it live then?
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 should probably go on
tokio::task
ortokio::runtime
is my thought.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 moved this into
tokio::task
and madetokio::coop
pub(crate)
again.