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

Task killing, linked failure, and exit code propagation in the new runtime. #7858

Closed
wants to merge 29 commits into from

Conversation

bblum
Copy link
Contributor

@bblum bblum commented Jul 17, 2013

Some notes about the commits.

Exit code propagation commits:

  • Reimplement unwrap() has the same old code from arc::unwrap ported to use modern atomic types and finally (it's considerably nicer this way)
  • Add try_unwrap() has some new slightly-tricky (but pretty simple) concurrency primitive code
  • Add KillHandle and Add kill::Death are the bulk of the logic.

Task killing commits:

  • Implement KillHandle::kill() and friends, Do a task-killed check, and Add BlockedTask implement the killing logic;
  • Change the HOF context switchers turns said logic on

Linked failure commits:

  • Replace *rust_task ptrs adapts the taskgroup code to work for both runtimes
  • Enable taskgroup code does what it says on the tin.

r? @brson

@bluss bluss mentioned this pull request Jul 18, 2013
@brson
Copy link
Contributor

brson commented Jul 18, 2013

It looks to me that because of this line there is an additional CAS on every deschedule now. Is that correct? So receives are now two atomics?

@brson
Copy link
Contributor

brson commented Jul 18, 2013

Same for send. A send that needs to wake another task is two atomics.

@brson
Copy link
Contributor

brson commented Jul 18, 2013

Oh, but I guess that's where unkillable tasks come in. An indestructable task is unkillable and doesn't do the extra atomics.

@brson
Copy link
Contributor

brson commented Jul 18, 2013

I'm r+ing this because I want to get the tests passing and the new runtime finished, but this model is too complicated. Now we have three different ways that tasks communicate failure where before we had two.

You told me you were going to remove the complexity of supervision and that you were going to combine 'watched' and 'linked', and you did not. We're going to revisit this in the future.

@brson
Copy link
Contributor

brson commented Jul 19, 2013

Here's a summary of my current concerns.

Complexity

The interface has a lot of options that it's not clear we need (linked, supervised, watched). I don't know that these are the semantics we want to commit to, nor that they can be explained easily. The addition of 'watched' mode makes the semantics more awkward.

Bookkeeping

Maintaining task groups requires a lot of book keeping, imposing several synchronizing operations on task spawning and termination. This bookkeeping involves taking a lock, and because by default there is a single task group, there is effectively a global lock on spawn/terminate.

Interruptable blocking and I/O

Even I/O is interruptable, but that means that all I/O will need to do thread synchronization.

Shutdown

This linkage model doesn't do anything to enable graceful runtime shutdown, and will require further bookkeeping to figure out when to tear down the schedulers.

@bblum
Copy link
Contributor Author

bblum commented Jul 20, 2013

Whoops. Nasty memory leak I forgot to write a case for in port's destructor. Thanks to valgrind and having enough tests that I caught it.

@bblum
Copy link
Contributor Author

bblum commented Jul 20, 2013

Though... I thought the runtime used to crash if the process exited with any leaked exchange allocations (i.e., not require valgrind to notice)? It doesn't seem to have done so here.

bors added a commit that referenced this pull request Jul 20, 2013
Some notes about the commits.

Exit code propagation commits:
* ```Reimplement unwrap()``` has the same old code from ```arc::unwrap``` ported to use modern atomic types and finally (it's considerably nicer this way)
* ```Add try_unwrap()``` has some new slightly-tricky (but pretty simple) concurrency primitive code
* ```Add KillHandle``` and ```Add kill::Death``` are the bulk of the logic.

Task killing commits:
* ```Implement KillHandle::kill() and friends```, ```Do a task-killed check```, and ```Add BlockedTask``` implement the killing logic;
* ```Change the HOF context switchers``` turns said logic on

Linked failure commits:
* ```Replace *rust_task ptrs``` adapts the taskgroup code to work for both runtimes
* ```Enable taskgroup code``` does what it says on the tin.

r? @brson
@bors bors closed this Jul 20, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 23, 2021
[`swap`] lints now check if there is `no_std` or `no_core` attribute

Closes rust-lang#7858

changelog: [`swap`] lints now check if there is `no_std` or `no_core` attribute
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.

3 participants