Unexpected drop

[kvark]

Consider the following program:

#[macro_use]
extern crate serde_derive;
extern crate ipc_channel;

#[derive(Serialize, Deserialize)]
struct Foo;

impl Drop for Foo {
    fn drop(&mut self) {
        println!("drop");
    }
}

fn main() {
    let (sender, receiver) = ipc_channel::ipc::channel().unwrap();
    let foo = Foo;
    sender.send(foo).unwrap(); //`foo` consumed here
    let _foo2 = receiver.recv().unwrap(); //`_foo2` is expected to drop here
}

From the user perspective, value gets consumed by sender and then reproduced by the receiver. This is expressed as move semantics by the API but doesn't actually behave this way: the program drops Foo twice here.

I understand that with serialization involved it becomes more difficult to say when the value is destroyed and when it's not. I do find this rather critical, since the user code may have unintended effects in Drop, and debugging cases like this is not fun. Can the implementation make sure to avoid dropping an object on send, and only drop it if the send failed? Alternatively, something should prevent/warn the user from ever having destructors on the types being sent.

cc @glennw

[antrik]

@kvark is there a specific situation where you are running into this problem? If so, let's discuss that first before considering the general case...

The pseudo-move semantics for ipc-channel are intended to allow its use as a more or less drop-in replacement for mpsc channels. Admittedly, these semantics are not always ideal for IPC; and there has been some discussion about adding other options. (See https://github.com/servo/ipc-channel/issues/138 etc.)

For the existing interface, I can see how dropping can be unexpected, if your Drop handler does things that affect the "world" across process boundaries. On the other hand, I suspect the majority of Drop handlers actually deal with things that are local to the process -- in which case we do want to drop... So I think users just need to be aware of the drop, and make sure they work around it when necessary. (Probably by wrapping the object in question in an Arc<>; cloning it before the send; and mem::forget() afterwards.)

[kvark]

@antrik thanks for taking your time to respond!

is there a specific situation where you are running into this problem?

I don't think the specifics are important here. The API mimics mpsc with a move semantics. The API ends up doing copy semantics, while allowing Drop, which reads like broken semantics to me.

In my case, the destructor was sending an IPC message (somewhat ironically?). So I ended up receiving a message twice with no apparent indicator of why. That wasted quite a bit of my time.

[antrik]

@kvark you are right that we aren't implementing perfect move semantics here. The problem is, as I already pointed out, that many objects do need to be destroyed when leaving the process -- so I don't think there is a solution that is correct in all situations, without discriminating per type.

I believe this ultimately boils down to a limitation in the Serde API: it always treats serialisation as a kind of deep-clone; passing only a shared reference to the type-specific serialize() method implementations. To solve this properly, we would need a way to do a "consuming" serialisation instead, where the implementations can decide individually whether to just let the object drop at the end, or to mem::forget() it. (Or maybe yet something different...)

This problem actually comes up in ipc-channel itself: OsIpcReceiver has to emulate a consuming serialisation, by wrapping the data in an interior mutability container; the serialize() method then uses this to unset the value, so the drop handler isn't invoked later on. This is a hack though, which breaks all uses of Serde on the type in question in contexts that do actually want the deep clone semantics...

(It might be possible to enhance this workaround so it only emulates move semantics in an IPC context -- I have a rough idea how to do that, which might or might not work -- but this would be even more hacky...)

It seems to me the only way to solve this properly is by extending Serde, to provide consuming serialisation officially as an option. I think this could probably be done in a pretty non-invasive fashion: in addition to the existing deep-cloning serialize() function, it could expose a consuming_serialize() or serialize_by_value() function, for any type that implements a corresponding ConsumingSerialize/SerializeByValue trait. Just like the existing Serialize trait, this would be available by default for all primitive types and containers (when the inner type has it in turn); and it could be auto-derived for aggregate types if all constituents have it; while other types would have to implement it manually. (Maybe the trait method could even have a default implementation that defers to the ordinary serialize() method, and only needs to be overridden for types that need special handling or want to optimise the move case? Not sure about the details...)

This actually sounds like a pretty interesting task. Unfortunately, I still have a lot of other stuff on my ToDo list for ipc-channel -- so it's not something I could work on myself any time soon...

[kvark]

many objects do need to be destroyed when leaving the process

Do you mean the standard library objects (Vec, Arc, etc) or the actual Servo's primitives?

If move semantics can't be safely provided, it shouldn't be exposed. Just drop the mpsc compatibility and have the API taking &object instead of consuming it.

[antrik]

@kvark I mean any object referencing process-local resources: be it memory (whether in standard containers or custom types), or system resources such as file descriptors.

The current semantics are quirky, but not unsafe. I totally agree that it would be good to fix them -- and I outlined a way to do so -- but I don't agree that it's so bad that it has to go away entirely.

As for the alternative API, it's a bit more complex than that, since some objects -- including OsIpcReceiver -- need move semantics to avoid other quirks. In https://github.com/servo/ipc-channel/issues/138 (especially the latest comment), I outlined an approach that should work reasonably well I believe -- it's again "just" a matter of implementing it...

[gterzian]

If move semantics can't be safely provided, it shouldn't be exposed. Just drop the mpsc compatibility and have the API taking &object instead of consuming it.

I had never though about it before, and to me having two drops in the above example makes sense. The "move vs copy" semantics are I think intra-process concepts, and so is Drop. So sending a message indeed moves it into the channel, where it is processed so that the data can then be sent over to another process. The "object" then drops, since it won't have a use in that process after that. The "copy" that is received on the other side is from my perspective not the result of Copy, it's the result of (de)serialization, and in that context it makes sense to think of the two objects as separate, each with their own drops.

Lastly, I think the move on send is not just about compat with mpsc. One could also imagine a send(&msg) mpsc API, however it's not one I would want to use. To me the "move" semantics in both cases are not about the underlying implementation details of cross-thread/process communication. I just wouldn't want to have code still able to do anything with msg after it has been sent, that's all. What "sent" means might differ, and the semantics appear the same to me.