Skip to content

Commit

Permalink
Don't make OsIpcSender Sync
Browse files Browse the repository at this point in the history
`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.
  • Loading branch information
antrik committed Nov 3, 2016
1 parent 77c0ca1 commit 20ed7d2
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 6 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -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"
Expand Down
10 changes: 5 additions & 5 deletions src/platform/inprocess/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}

Expand All @@ -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(()),
}
Expand Down
9 changes: 9 additions & 0 deletions src/platform/macos/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -334,6 +340,7 @@ impl Clone for OsIpcSender {
}
OsIpcSender {
port: self.port,
nosync_marker: PhantomData,
}
}
}
Expand All @@ -342,6 +349,7 @@ impl OsIpcSender {
fn from_name(port: mach_port_t) -> OsIpcSender {
OsIpcSender {
port: port,
nosync_marker: PhantomData,
}
}

Expand Down Expand Up @@ -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,
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/platform/unix/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand All @@ -139,6 +146,7 @@ impl OsIpcSender {
fn from_fd(fd: c_int) -> OsIpcSender {
OsIpcSender {
fd: Arc::new(SharedFileDescriptor(fd)),
nosync_marker: PhantomData,
}
}

Expand Down

0 comments on commit 20ed7d2

Please sign in to comment.