Implement Sync for IpcSender (#415)

* Implement Sync for `IpcSender`

Our queue is a multi-producer queue, which by definition must
support concurrent sending.
Both crossbeam and the std::sync::mpsc queue (since Rust 1.72) implement Sync, hence
there is no reason not to implement this anymore.

Related zulip discussion: https://servo.zulipchat.com/#narrow/channel/263398-general/topic/Implement.20Sync.20for.20.60IpcSender.60/with/539930964

* fix unused_import

Author: jschwe

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ipc-channel"
-version = "0.20.1"
+version = "0.20.2"
 description = "A multiprocess drop-in replacement for Rust channels"
 authors = ["The Servo Project Developers"]
 license = "MIT OR Apache-2.0"

src/platform/inprocess/mod.rs

@@ -21,7 +21,6 @@ use std::ptr::eq;
 use std::slice;
 use std::sync::{Arc, LazyLock, Mutex};
 use std::time::Duration;
-use usize;
 use uuid::Uuid;
 
 #[derive(Clone)]
@@ -124,23 +123,19 @@ impl OsIpcReceiver {
 
 #[derive(Clone, Debug)]
 pub struct OsIpcSender {
-    sender: RefCell<Sender<ChannelMessage>>,
+    sender: Sender<ChannelMessage>,
 }
 
 impl PartialEq for OsIpcSender {
     fn eq(&self, other: &OsIpcSender) -> bool {
-        eq(
-            &*self.sender.borrow() as *const _,
-            &*other.sender.borrow() as *const _,
-        )
+        // FIXME: this implementation is wrong: https://github.com/servo/ipc-channel/issues/414
+        eq(&self.sender as *const _, &other.sender as *const _)
     }
 }
 
 impl OsIpcSender {
     fn new(sender: Sender<ChannelMessage>) -> OsIpcSender {
-        OsIpcSender {
-            sender: RefCell::new(sender),
-        }
+        OsIpcSender { sender }
     }
 
     pub fn connect(name: String) -> Result<OsIpcSender, ChannelError> {
@@ -162,7 +157,6 @@ impl OsIpcSender {
         let os_ipc_channels = ports.into_iter().map(OsOpaqueIpcChannel::new).collect();
         let ipc_message = IpcMessage::new(data.to_vec(), os_ipc_channels, shared_memory_regions);
         self.sender
-            .borrow()
             .send(ChannelMessage(ipc_message))
             .map_err(|_| ChannelError::BrokenPipeError)
     }

src/platform/macos/mod.rs

@@ -23,7 +23,6 @@ use std::error::Error as StdError;
 use std::ffi::CString;
 use std::fmt::{self, Debug, Formatter};
 use std::io;
-use std::marker::PhantomData;
 use std::mem;
 use std::ops::Deref;
 use std::ptr;
@@ -392,12 +391,6 @@ impl<'a> SendData<'a> {
 #[derive(PartialEq, Debug)]
 pub struct OsIpcSender {
     port: mach_port_t,
-    // Make sure this is `!Sync`, to match `crossbeam_channel::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 {
@@ -419,19 +412,13 @@ impl Clone for OsIpcSender {
                 Err(error) => panic!("mach_port_mod_refs(1, {}) failed: {:?}", cloned_port, error),
             }
         }
-        OsIpcSender {
-            port: cloned_port,
-            nosync_marker: PhantomData,
-        }
+        OsIpcSender { port: cloned_port }
     }
 }
 
 impl OsIpcSender {
     fn from_name(port: mach_port_t) -> OsIpcSender {
-        OsIpcSender {
-            port,
-            nosync_marker: PhantomData,
-        }
+        OsIpcSender { port }
     }
 
     pub fn connect(name: String) -> Result<OsIpcSender, MachError> {
@@ -597,7 +584,6 @@ 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

@@ -1098,11 +1098,18 @@ fn try_recv_large_delayed() {
 
 mod sync_test {
     use crate::platform;
-    use static_assertions::assert_not_impl_any;
+    use static_assertions::{assert_impl_all, assert_not_impl_any};
 
     #[test]
     fn receiver_not_sync() {
-        assert_not_impl_any!(platform::OsIpcSender : Sync);
+        // A single-consumer queue is not Sync.
+        assert_not_impl_any!(platform::OsIpcReceiver : Sync);
+    }
+
+    #[test]
+    fn sender_is_sync() {
+        // A multi-producer queue must be Sync.
+        assert_impl_all!(platform::OsIpcSender : Sync);
     }
 }
 

src/platform/unix/mod.rs

@@ -29,7 +29,6 @@ use std::ffi::{c_uint, CString};
 use std::fmt::{self, Debug, Formatter};
 use std::hash::BuildHasherDefault;
 use std::io;
-use std::marker::PhantomData;
 use std::mem;
 use std::ops::{Deref, RangeFrom};
 use std::os::fd::RawFd;
@@ -185,18 +184,12 @@ impl Drop for SharedFileDescriptor {
 #[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 OsIpcSender {
     fn from_fd(fd: c_int) -> OsIpcSender {
         OsIpcSender {
             fd: Arc::new(SharedFileDescriptor(fd)),
-            nosync_marker: PhantomData,
         }
     }
 

src/platform/windows/mod.rs

@@ -12,13 +12,13 @@ use bincode;
 use serde;
 
 use std::{
-    cell::{Cell, RefCell},
+    cell::RefCell,
     cmp::PartialEq,
     convert::TryInto,
     env,
     ffi::CString,
     fmt, io,
-    marker::{PhantomData, Send, Sync},
+    marker::{Send, Sync},
     mem,
     ops::{Deref, DerefMut, RangeFrom},
     ptr, slice,
@@ -1291,11 +1291,6 @@ impl OsIpcReceiver {
 #[derive(Debug, PartialEq)]
 pub struct OsIpcSender {
     handle: WinHandle,
-    // 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 Clone for OsIpcSender {
@@ -1315,10 +1310,7 @@ impl OsIpcSender {
     }
 
     fn from_handle(handle: WinHandle) -> OsIpcSender {
-        OsIpcSender {
-            handle,
-            nosync_marker: PhantomData,
-        }
+        OsIpcSender { handle }
     }
 
     /// Connect to a pipe server.

src/test.rs

@@ -750,3 +750,20 @@ fn test_receiver_stream() {
         _ => panic!("Stream should have 5"),
     };
 }
+
+mod sync_test {
+    use crate::ipc::{IpcReceiver, IpcSender};
+    use static_assertions::{assert_impl_all, assert_not_impl_any};
+
+    #[test]
+    fn ipc_receiver_not_sync() {
+        // A single-consumer queue is not Sync.
+        assert_not_impl_any!(IpcReceiver<usize> : Sync);
+    }
+
+    #[test]
+    fn ipc_sender_is_sync() {
+        // A multi-producer queue must be Sync.
+        assert_impl_all!(IpcSender<usize> : Sync);
+    }
+}

tests/integration_test.rs

@@ -15,7 +15,7 @@ use std::{env, process};
 // These integration tests may be run on their own by issuing:
 // cargo test --test '*'
 
-/// Test spawing a process which then acts as a client to a
+/// Test spawning a process which then acts as a client to a
 /// one-shot server in the parent process.
 #[cfg(not(any(feature = "force-inprocess", target_os = "android", target_os = "ios")))]
 #[test]