Auto merge of #115 - antrik:nosync_sender, r=pcwalton

Don't make `OsIpcSender` `Sync`

`mpsc::Sender` isn't `Sync` -- and the `ipc-channel` equivalent probably
shouldn't be either. Sharing references to senders is usually not a good
idea: they are rather meant to be cloned instead; and they implement
internal sharing to make this efficient and robust -- sharing references
to senders just adds an unnecessary extra layer of sharing on top.

For the `inprocess` back-end, implementing `Sync` was necessitating
use of an `Arc<Mutex<>>`, adding considerable overhead even when not
used -- so this change is an optimisation. It effectively reverts most
of the sender part of 30b2024 (the
receiver part was already backed out in
77469c2 ), except that it obviously
doesn't re-introduce the bogus explicit `Sync` claim.

For the `macos` and `unix` back-ends, the sender implementations were
inherently `Sync`; so we explicitly mark them `!Sync` now, to get a
consistent API.

Also bumping the `ipc-channel` version here, as this is a breaking
change.

(It affects Servo only in one place though, see servo/servo#14048 )

This also comes with a test case to verify that `OsIpcSender` indeed isn't `Sync` any more; and a CI change to activate it.

.travis.yml

@@ -8,12 +8,12 @@ os:
   - osx
 
 env:
-  - FEATURES=""
-  - FEATURES="--features force-inprocess"
+  - FEATURES="unstable"
+  - FEATURES="unstable force-inprocess"
 
 notifications:
   webhooks: http://build.servo.org:54856/travis
 
 script:
-  - cargo build $FEATURES
-  - cargo test $FEATURES
+  - cargo build --features "$FEATURES"
+  - cargo test --features "$FEATURES"

Cargo.toml

@@ -1,13 +1,14 @@
 [package]
 name = "ipc-channel"
-version = "0.5.1"
+version = "0.6.0"
 description = "A multiprocess drop-in replacement for Rust channels"
 authors = ["The Servo Project Developers"]
 license = "MIT/Apache-2.0"
 repository = "https://github.com/servo/ipc-channel"
 
 [features]
 force-inprocess = []
+unstable = []
 
 [dependencies]
 bincode = "0.6"

appveyor.yml

@@ -13,4 +13,4 @@ install:
 build: false
 
 test_script:
-  - cargo test --verbose
+  - 'cargo test --verbose --features "unstable"'

src/lib.rs

@@ -9,6 +9,7 @@
 
 #![cfg_attr(any(feature = "force-inprocess", target_os = "windows", target_os = "android"),
 			feature(mpsc_select))]
+#![cfg_attr(all(feature = "unstable", test), feature(specialization))]
 
 #[macro_use]
 extern crate lazy_static;

src/platform/inprocess/mod.rs

@@ -106,20 +106,20 @@ impl OsIpcReceiver {
 
 #[derive(Clone, Debug)]
 pub struct OsIpcSender {
-    sender: Arc<Mutex<mpsc::Sender<MpscChannelMessage>>>,
+    sender: RefCell<mpsc::Sender<MpscChannelMessage>>,
 }
 
 impl PartialEq for OsIpcSender {
     fn eq(&self, other: &OsIpcSender) -> bool {
-        &*self.sender.lock().unwrap() as *const _ ==
-            &*other.sender.lock().unwrap() as *const _
+        &*self.sender.borrow() as *const _ ==
+            &*other.sender.borrow() as *const _
     }
 }
 
 impl OsIpcSender {
     fn new(sender: mpsc::Sender<MpscChannelMessage>) -> OsIpcSender {
         OsIpcSender {
-            sender: Arc::new(Mutex::new(sender)),
+            sender: RefCell::new(sender),
         }
     }
 
@@ -139,7 +139,7 @@ impl OsIpcSender {
                 shared_memory_regions: Vec<OsIpcSharedMemory>)
                 -> Result<(),MpscError>
     {
-        match self.sender.lock().unwrap().send(MpscChannelMessage(data.to_vec(), ports, shared_memory_regions)) {
+        match self.sender.borrow().send(MpscChannelMessage(data.to_vec(), ports, shared_memory_regions)) {
             Err(_) => Err(MpscError::ChannelClosedError),
             Ok(_) => Ok(()),
         }

src/platform/macos/mod.rs

@@ -19,6 +19,7 @@ use std::cell::Cell;
 use std::ffi::CString;
 use std::fmt::{self, Debug, Formatter};
 use std::io::{Error, ErrorKind};
+use std::marker::PhantomData;
 use std::mem;
 use std::ops::Deref;
 use std::ptr;
@@ -306,6 +307,11 @@ impl OsIpcReceiver {
 #[derive(PartialEq, Debug)]
 pub struct OsIpcSender {
     port: mach_port_t,
+    // Make sure this is `!Sync`, to match `mpsc::Sender`; and to discourage sharing references.
+    //
+    // (Rather, senders should just be cloned, as they are shared internally anyway --
+    // another layer of sharing only adds unnecessary overhead...)
+    nosync_marker: PhantomData<Cell<()>>,
 }
 
 impl Drop for OsIpcSender {
@@ -334,6 +340,7 @@ impl Clone for OsIpcSender {
         }
         OsIpcSender {
             port: self.port,
+            nosync_marker: PhantomData,
         }
     }
 }
@@ -342,6 +349,7 @@ impl OsIpcSender {
     fn from_name(port: mach_port_t) -> OsIpcSender {
         OsIpcSender {
             port: port,
+            nosync_marker: PhantomData,
         }
     }
 
@@ -480,6 +488,7 @@ impl OsOpaqueIpcChannel {
     pub fn to_sender(&mut self) -> OsIpcSender {
         OsIpcSender {
             port: mem::replace(&mut self.port, MACH_PORT_NULL),
+            nosync_marker: PhantomData,
         }
     }
 

src/platform/test.rs

@@ -629,3 +629,27 @@ fn try_recv_large_delayed() {
         thread.join().unwrap();
     }
 }
+
+#[cfg(feature = "unstable")]
+mod sync_test {
+    use platform;
+
+    trait SyncTest {
+        fn test_not_sync();
+    }
+
+    impl<T> SyncTest for T {
+        default fn test_not_sync() {}
+    }
+
+    impl<T: Sync> SyncTest for T {
+        fn test_not_sync() {
+            panic!("`OsIpcSender` should not be `Sync`");
+        }
+    }
+
+    #[test]
+    fn receiver_not_sync() {
+        platform::OsIpcSender::test_not_sync();
+    }
+}

src/platform/unix/mod.rs

@@ -12,11 +12,13 @@ use libc::{self, MAP_FAILED, MAP_SHARED, POLLIN, PROT_READ, PROT_WRITE, SOCK_SEQ
 use libc::{SO_LINGER, S_IFMT, S_IFSOCK, c_char, c_int, c_void, getsockopt};
 use libc::{iovec, mkstemp, mode_t, msghdr, nfds_t, off_t, poll, pollfd, recvmsg, sendmsg};
 use libc::{setsockopt, size_t, sockaddr, sockaddr_un, socketpair, socklen_t, sa_family_t};
+use std::cell::Cell;
 use std::cmp;
 use std::collections::HashSet;
 use std::ffi::{CStr, CString};
 use std::fmt::{self, Debug, Formatter};
 use std::io::Error;
+use std::marker::PhantomData;
 use std::mem;
 use std::ops::Deref;
 use std::ptr;
@@ -124,6 +126,11 @@ pub struct SharedFileDescriptor(c_int);
 #[derive(PartialEq, Debug, Clone)]
 pub struct OsIpcSender {
     fd: Arc<SharedFileDescriptor>,
+    // Make sure this is `!Sync`, to match `mpsc::Sender`; and to discourage sharing references.
+    //
+    // (Rather, senders should just be cloned, as they are shared internally anyway --
+    // another layer of sharing only adds unnecessary overhead...)
+    nosync_marker: PhantomData<Cell<()>>,
 }
 
 impl Drop for SharedFileDescriptor {
@@ -139,6 +146,7 @@ impl OsIpcSender {
     fn from_fd(fd: c_int) -> OsIpcSender {
         OsIpcSender {
             fd: Arc::new(SharedFileDescriptor(fd)),
+            nosync_marker: PhantomData,
         }
     }