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

RFC: Task spawn options interface #2954

Closed
bblum opened this issue Jul 18, 2012 · 12 comments
Closed

RFC: Task spawn options interface #2954

bblum opened this issue Jul 18, 2012 · 12 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one.

Comments

@bblum
Copy link
Contributor

bblum commented Jul 18, 2012

A couple weeks ago we discussed being able to 'chain' task spawn options, which would be much more fun to write than the current builder interface.

I envision:

fn unlinked() -> builder { ... }
fn notify_chan() -> builder { ... }
fn spawn() { ... }

impl for builder {
    fn unlinked() -> builder {
        { link: false with builder }
    }
    fn notify_chan(chan) -> builder 
    ...
    fn spawn() ...
}

With the functions at the top level returning a default set. Then you'll never need to type "builder" - all of these will be valid:

do task::spawn { ... }
do task::unlinked().notify_chan(c).spawn { ... }
do task::notify_chan(c).unlinked().spawn { ... }
let t = task::unlinked();
do t.spawn { ... }
do t.notify_chan(c).spawn { ... }

One problem is noncopyability of certain things that might get put in builder - like wrappers and notify ports (or future stuff...? not entirely clear on that).

Move mode on self can solve that; until then, we could perhaps make it a runtime error by doing the standard option dance. (When move mode on self appears, we can make it a build error without changing the interface.)

type builder = ~mut option<{...record...}>
impl for builder {
    fn set_noncopyable_thing() -> builder {
        let mut result = none;
        result <-> *self;
    }
}

An attempt to use a builder with none in it would fail at runtime.

@ghost ghost assigned bblum Jul 18, 2012
@eholk
Copy link
Contributor

eholk commented Jul 18, 2012

It'd be cool if notify_chan used a pipe protocol instead of the channels and ports we have now.

/me shamelessly wants to see his feature used more :)

@graydon
Copy link
Contributor

graydon commented Jul 18, 2012

I've no strong preference on this API. So long as the default is easy to type and the variants can all be specified in O(number-of-non-defaults) worth of work, I don't really care if we use FRU, this sort of "fluent" OO thing, an optional list of enums ... whatever.

I'm slightly less fond of the name duplication you get by omitting the "chain start" call in this style, mind you. Might prefer task::cfg().unsupervised().notify_chan().spawn or tast::build().unsupervised()... or such. But it's just a minor preference.

@bblum
Copy link
Contributor Author

bblum commented Jul 18, 2012

@graydon In this case I think I lean towards the most absurdly-over-the-top-easy-to-type thing. How about task::t() for the builder?

@graydon
Copy link
Contributor

graydon commented Jul 18, 2012

I suppose. Don't personally care a lot. Though I think people will complain. Should probably be something with a little more meaning. Setup?

@bblum
Copy link
Contributor Author

bblum commented Jul 18, 2012

"task::task()"? (edit: argh, it won't show up in code format if it's the only thing)

@bblum
Copy link
Contributor Author

bblum commented Jul 18, 2012

Also, taskgroup handles can probably fit easily here:

task::task().linked().spawn() // Its own group, whose handle we never see
let tg = task::taskgroup();
task::task().linked_to(tg).spawn() // Explicit handle management

@nikomatsakis
Copy link
Contributor

Probably the reason that move-on-self got mentioned is that some of the options create ports and so forth and those would only be valid for one spawn. Nothing in this API prevents you from doing something like:

let x = task::task().joinable();
do x.spawn { }
do x.spawn { }

I believe joinable was the example that would cause a problem. I am not sure that the old API prevented this condition either, though: I know @brson wanted to use a resource to prevent copying at some point, but I can't remember if we decided to just say "don't do that".

@bblum
Copy link
Contributor Author

bblum commented Jul 19, 2012

builders are currently noncopyable, by means of a dummy port<()>. i think i'd like to get rid of that, in hopes of move mode on self, and use an option with runtime-copy-failure as a holdover.

@eholk
Copy link
Contributor

eholk commented Jul 19, 2012

If we want builder to be noncopyable, why not just make it a class with a destructor?

@bblum
Copy link
Contributor Author

bblum commented Jul 19, 2012

eventually it should be noncopyable, but in order to do the chaining thing without move-self it needs to be copyable for now (or at least reconstructable). something like:

fn set_noncopyable_2(x: thing) -> builder {
    assert !self.consumed;
    self.consumed = true;
    let mut tmp = none;
    self.noncopyable_1 <-> tmp;
    { consumed: false, noncopyable_1: tmp, noncopyable_2: x with self }
}

@bblum
Copy link
Contributor Author

bblum commented Jul 19, 2012

Also I think there should be some default functions that don't require you to go through even this more-minimal builder interface. task::spawn should work exactly as before. maybe some others? task::spawn_unlinked (for no-way propagation) and task::spawn_supervised (for one-way propagation)?

don't wanna get too heavy with these, but don't want to require the builder interface in the majority of cases either.

@bblum
Copy link
Contributor Author

bblum commented Jul 24, 2012

This is implemented.

@bblum bblum closed this as completed Jul 24, 2012
@bblum bblum removed their assignment Jun 16, 2014
RalfJung pushed a commit to RalfJung/rust that referenced this issue Jul 8, 2023
cron auto-PR: can't seem to avoid repeating the branch name...
celinval pushed a commit to celinval/rust-dev that referenced this issue Jun 4, 2024
The "CBMC latest" workflow is composed of two jobs (`regression` and
`perf`) which perform testing with the most recent development version
of CBMC. At present, the `regression` jobs are not actually testing with
the CBMC that we build from source, but the CBMC installed through the
setup scripts, as revealed in rust-lang#2954.

This PR changes the `regression` jobs so that they use `cmake` to build.
This allows the runner to pick up the recently-built CBMC development
version instead of the one installed through setup scripts, as it's done
in the `perf` jobs. Unfortunately, [this CI
run](https://github.com/adpaco-aws/rmc/actions/runs/7390380572) doesn't
demonstrate the fix as it should due to an unrelated breaking change in
the latest CBMC version. However, rust-lang#2952 provides more context in case
you need it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one.
Projects
None yet
Development

No branches or pull requests

4 participants