Skip to content

Conversation

@Munksgaard
Copy link
Owner

Introduce destructor-bombs to improve the (runtime) safety of the library.

@Manishearth
Copy link
Collaborator

It may be possible to avoid using the extra bool field by prematurely running drop (manually) in close(), like so

This means we would have to do the following in close():

fn close(mut self) {
 // create some dummy values to place inside
 // this is safe because nobody will read these
 let mut sender = unsafe { mem::uninitialized() };
 let mut receiver = unsafe { mem::uninitialized() };
 // extract the internal sender/receiver so that we can drop them
 mem::swap(&mut self.0, &mut sender);
 mem::swap(&mut self.1, &mut receiver);

 drop(sender);drop(receiver); // drop them
 mem::forget(self); // Ensure destructors don't run
}

The implementation of mem::swap uses the same combination of creating an uninitialized() scratch space and forgeting it before it can get accessed.

Sadly, we cannot move out of types implementing destructors, otherwise the above impl could just destructure and drop the components. Instead, we have to do some trickery.

@Munksgaard
Copy link
Owner Author

That is certainly another possibility. It keeps the Chan implementation simple, but perhaps your implementation of close would throw some people off? Also, do you have any ideas what the performance would be like?

@Manishearth
Copy link
Collaborator

Just leave an elaborate comment and it should be fine.

It's not inefficient; we do the same thing drop() does (without calling Drop::drop()), and we additionally allocate stack space for a dummy sender/receiver pair. Not heavy at all.

@Munksgaard
Copy link
Owner Author

Cool. Do you wanna do a PR?

Manishearth and others added 3 commits September 2, 2015 20:31
The documentation tests were not correctly closing all of their
channels, and with the newly introduced destructor bombs they fail
horribly. This fixes the tests, but the result turns out to be
exceptionally ugly, and we should definitely try to come up with some
better examples that showcase the `chan_select!` macros without being
horribly verbose about closing channels.

cc: @laumann
@Manishearth
Copy link
Collaborator

Updated this one

@Munksgaard
Copy link
Owner Author

Before we merge this, I think we should make sure that it wont hinder @laumann's port of Servo too much (https://github.com/laumann/servo/tree/session-types). Obviously, destructor-bombs like these shouldn't have an impact on correctly designed programs, but because the Servo port is the main use case of this library so far, it'd be nice to actually see it work before we merge.

@laumann
Copy link
Collaborator

laumann commented Sep 2, 2015

In the Servo code I've taken great pains to properly close channels (and have considered it an error when this didn't happen).

EDIT: But yes, I agree that it would be nice to see it working before we go all in...

@Manishearth
Copy link
Collaborator

That can't stop the destructor from running. Moving it to compile time requires linear types, which Rust doesn't have (not counting humpty_dumpty, which is an approximation)

@Manishearth
Copy link
Collaborator

The thing is, even with a protocol-complete marker, you can't force folks to reach that marker in highly concurrent code (we can't just wrap it in a function like above because the protocol may be enacted over many steps via structs and stuff, and branch off).

@Munksgaard
Copy link
Owner Author

Hi @setir, thanks for contributing 😄

That is definitely an interesting idea, and one that was somewhat suggested at our recent presentation of the library at WGP.

As I understand it, your idea is somewhat similar to having a function like the following

fn start_session<P: HasDual>(Fn(P) -> Eps, Fn(P::Dual) -> Eps)

instead of the session_channel() function that we provide. That is, a function that takes two functions as input, and runs them as the opposite sides of a session, requiring that both return Eps (or Completed).

I agree that this would ensure that both protocols run to completion, but I believe the lack of flexibility would cause some problems for us. For instance, It would become much harder to set up more complex communication networks: How would you set up a network consisting of three processes all talking to each other using only start_session? The way I see it, you would almost certainly need to rely on other primitives, like Rust's builtin channels, because start_session only supports two processes at a time.

@Munksgaard
Copy link
Owner Author

@laumann: Let's merge this as well, shall we?

Munksgaard added a commit that referenced this pull request Nov 19, 2015
@Munksgaard Munksgaard merged commit 5c5d680 into master Nov 19, 2015
@Munksgaard Munksgaard deleted the destructo-bomb branch November 19, 2015 12:14
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.

4 participants