From 20ed7d2472977f0eb69a524d1f0b37b09f54d31d Mon Sep 17 00:00:00 2001 From: Olaf Buddenhagen Date: Tue, 1 Nov 2016 22:26:47 +0100 Subject: [PATCH 1/3] 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>`, adding considerable overhead even when not used -- so this change is an optimisation. It effectively reverts most of the sender part of 30b20244dc83e61f9ba09b6e3db13a8ff7ac4665 (the receiver part was already backed out in 77469c2ed06ee51cb3a5bac9dbac4e7f9f57c2d2 ), 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. --- Cargo.toml | 2 +- src/platform/inprocess/mod.rs | 10 +++++----- src/platform/macos/mod.rs | 9 +++++++++ src/platform/unix/mod.rs | 8 ++++++++ 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 88b4be9fb..b6366aa81 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [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" diff --git a/src/platform/inprocess/mod.rs b/src/platform/inprocess/mod.rs index 903c2f895..10c2ca0b9 100644 --- a/src/platform/inprocess/mod.rs +++ b/src/platform/inprocess/mod.rs @@ -106,20 +106,20 @@ impl OsIpcReceiver { #[derive(Clone, Debug)] pub struct OsIpcSender { - sender: Arc>>, + sender: RefCell>, } 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) -> OsIpcSender { OsIpcSender { - sender: Arc::new(Mutex::new(sender)), + sender: RefCell::new(sender), } } @@ -139,7 +139,7 @@ impl OsIpcSender { shared_memory_regions: Vec) -> 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(()), } diff --git a/src/platform/macos/mod.rs b/src/platform/macos/mod.rs index 9d5504664..d6bbe2713 100644 --- a/src/platform/macos/mod.rs +++ b/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>, } 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, } } diff --git a/src/platform/unix/mod.rs b/src/platform/unix/mod.rs index 1383bb389..fa3273da3 100644 --- a/src/platform/unix/mod.rs +++ b/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, + // 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>, } 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, } } From f8800b7e32a3da314b5eb10822db05622879816f Mon Sep 17 00:00:00 2001 From: Olaf Buddenhagen Date: Thu, 3 Nov 2016 11:47:03 +0100 Subject: [PATCH 2/3] Tests: Add a test case to verify that `OsIpcSender` is `!Sync` now Unfortunately this requires an "unstable" feature to be enabled, because it relies on specialization... Can't think of any other way to handle this. --- Cargo.toml | 1 + src/lib.rs | 1 + src/platform/test.rs | 24 ++++++++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index b6366aa81..4715bc378 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -8,6 +8,7 @@ repository = "https://github.com/servo/ipc-channel" [features] force-inprocess = [] +unstable = [] [dependencies] bincode = "0.6" diff --git a/src/lib.rs b/src/lib.rs index 200c95be6..35e3daad7 100644 --- a/src/lib.rs +++ b/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; diff --git a/src/platform/test.rs b/src/platform/test.rs index ec3fe0eaa..cb9319fb2 100644 --- a/src/platform/test.rs +++ b/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 SyncTest for T { + default fn test_not_sync() {} + } + + impl SyncTest for T { + fn test_not_sync() { + panic!("`OsIpcSender` should not be `Sync`"); + } + } + + #[test] + fn receiver_not_sync() { + platform::OsIpcSender::test_not_sync(); + } +} From ad19e553d3e03c12dc5249f730fdcb94e358d37d Mon Sep 17 00:00:00 2001 From: Olaf Buddenhagen Date: Thu, 3 Nov 2016 12:12:16 +0100 Subject: [PATCH 3/3] CI: Build with `--features unstable`, to enable `!Sync` test --- .travis.yml | 8 ++++---- appveyor.yml | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index bf0861205..c0caaf5ad 100644 --- a/.travis.yml +++ b/.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" diff --git a/appveyor.yml b/appveyor.yml index fdece3593..4b80b86bd 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -13,4 +13,4 @@ install: build: false test_script: - - cargo test --verbose + - 'cargo test --verbose --features "unstable"'