Skip to content

Commit

Permalink
inprocess: Don't wrap OsIpcReceiver to make it Sync
Browse files Browse the repository at this point in the history
Wrap the inprocess receiver in a simple `RefCell<>` instead of
`Arc<Mutex<>>` (again).

Adding the mutex here is not a good idea: it only adds overhead and
potential errors, since not all of the other backends have a `Sync`
receiver either; nor does the `mpsc` mechanism `ipc-channel` is modelled
after. In fact it fundamentally violates the "single receiver" aspect of
`mpsc`, as outlined in
servo#89 (comment)

Users can still wrap it explicitly if they need to, and know it doesn't
introduce incorrect behaviour in the specific use case.

This change mostly reverts the `Receiver` part of
30b2024 ; it doesn't re-introduce the
explicit `Sync` declaration for `Receiver` though, as that was/is
clearly not true without the Mutex -- nor really desirable, as explained
above. This change also doesn't touch the `Sender` part, which is a
different story.
  • Loading branch information
antrik committed Oct 4, 2016
1 parent ca63fdb commit 77469c2
Showing 1 changed file with 9 additions and 9 deletions.
18 changes: 9 additions & 9 deletions src/platform/inprocess/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,13 @@ pub fn channel() -> Result<(OsIpcSender, OsIpcReceiver),MpscError> {
}

pub struct OsIpcReceiver {
receiver: Arc<Mutex<Option<mpsc::Receiver<MpscChannelMessage>>>>,
receiver: RefCell<Option<mpsc::Receiver<MpscChannelMessage>>>,
}

impl PartialEq for OsIpcReceiver {
fn eq(&self, other: &OsIpcReceiver) -> bool {
self.receiver.lock().unwrap().as_ref().map(|rx| rx as *const _) ==
other.receiver.lock().unwrap().as_ref().map(|rx| rx as *const _)
self.receiver.borrow().as_ref().map(|rx| rx as *const _) ==
other.receiver.borrow().as_ref().map(|rx| rx as *const _)
}
}

Expand All @@ -81,17 +81,17 @@ impl fmt::Debug for OsIpcReceiver {
impl OsIpcReceiver {
fn new(receiver: mpsc::Receiver<MpscChannelMessage>) -> OsIpcReceiver {
OsIpcReceiver {
receiver: Arc::new(Mutex::new(Some(receiver))),
receiver: RefCell::new(Some(receiver)),
}
}

pub fn consume(&self) -> OsIpcReceiver {
let receiver = self.receiver.lock().unwrap().take();
let receiver = self.receiver.borrow_mut().take();
OsIpcReceiver::new(receiver.unwrap())
}

pub fn recv(&self) -> Result<(Vec<u8>, Vec<OsOpaqueIpcChannel>, Vec<OsIpcSharedMemory>),MpscError> {
let r = self.receiver.lock().unwrap();
let r = self.receiver.borrow();
match r.as_ref().unwrap().recv() {
Ok(MpscChannelMessage(d,c,s)) => Ok((d,
c.into_iter().map(OsOpaqueIpcChannel::new).collect(),
Expand All @@ -101,7 +101,7 @@ impl OsIpcReceiver {
}

pub fn try_recv(&self) -> Result<(Vec<u8>, Vec<OsOpaqueIpcChannel>, Vec<OsIpcSharedMemory>),MpscError> {
let r = self.receiver.lock().unwrap();
let r = self.receiver.borrow();
match r.as_ref().unwrap().try_recv() {
Ok(MpscChannelMessage(d,c,s)) => Ok((d,
c.into_iter().map(OsOpaqueIpcChannel::new).collect(),
Expand Down Expand Up @@ -194,7 +194,7 @@ impl OsIpcReceiverSet {
let mut handles: Vec<mpsc::Handle<MpscChannelMessage>> = Vec::with_capacity(self.receivers.len());

for r in &self.receivers {
let inner_r = mem::replace(&mut *r.receiver.lock().unwrap(), None);
let inner_r = mem::replace(&mut *r.receiver.borrow_mut(), None);
receivers.push(inner_r);
}

Expand All @@ -218,7 +218,7 @@ impl OsIpcReceiverSet {

// put the receivers back
for (index,r) in self.receivers.iter().enumerate() {
mem::replace(&mut *r.receiver.lock().unwrap(), mem::replace(&mut receivers[index], None));
mem::replace(&mut *r.receiver.borrow_mut(), mem::replace(&mut receivers[index], None));
}

if r_id == -1 {
Expand Down

0 comments on commit 77469c2

Please sign in to comment.