Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support empty IpcSharedMemory #335

Merged
merged 3 commits into from
May 31, 2024
Merged

Support empty IpcSharedMemory #335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7f292d
Add tests for empty shared mem
sagudev May 4, 2024
3721b79
Simple empty slice
sagudev May 4, 2024
affa99e
Bump version to 0.18.1
sagudev May 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "ipc-channel"
version = "0.18.0"
version = "0.18.1"
description = "A multiprocess drop-in replacement for Rust channels"
authors = ["The Servo Project Developers"]
license = "MIT OR Apache-2.0"
Expand Down
83 changes: 57 additions & 26 deletions src/ipc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,15 +553,20 @@ impl IpcReceiverSet {
derive(PartialEq)
)]
pub struct IpcSharedMemory {
os_shared_memory: OsIpcSharedMemory,
/// None represents no data (empty slice)
os_shared_memory: Option<OsIpcSharedMemory>,
}

impl Deref for IpcSharedMemory {
type Target = [u8];

#[inline]
fn deref(&self) -> &[u8] {
&self.os_shared_memory
if let Some(os_shared_memory) = &self.os_shared_memory {
os_shared_memory
} else {
&[]
}
}
}

Expand All @@ -571,16 +576,22 @@ impl<'de> Deserialize<'de> for IpcSharedMemory {
D: Deserializer<'de>,
{
let index: usize = Deserialize::deserialize(deserializer)?;
let os_shared_memory = OS_IPC_SHARED_MEMORY_REGIONS_FOR_DESERIALIZATION.with(
|os_ipc_shared_memory_regions_for_deserialization| {
// FIXME(pcwalton): This could panic if the data was corrupt and the index was out
// of bounds. We should return an `Err` result instead.
os_ipc_shared_memory_regions_for_deserialization.borrow_mut()[index]
.take()
.unwrap()
},
);
Ok(IpcSharedMemory { os_shared_memory })
if index == usize::MAX {
Ok(IpcSharedMemory::empty())
} else {
let os_shared_memory = OS_IPC_SHARED_MEMORY_REGIONS_FOR_DESERIALIZATION.with(
|os_ipc_shared_memory_regions_for_deserialization| {
// FIXME(pcwalton): This could panic if the data was corrupt and the index was out
// of bounds. We should return an `Err` result instead.
os_ipc_shared_memory_regions_for_deserialization.borrow_mut()[index]
.take()
.unwrap()
},
);
Ok(IpcSharedMemory {
os_shared_memory: Some(os_shared_memory),
})
}
}
}

Expand All @@ -589,32 +600,52 @@ impl Serialize for IpcSharedMemory {
where
S: Serializer,
{
let index = OS_IPC_SHARED_MEMORY_REGIONS_FOR_SERIALIZATION.with(
|os_ipc_shared_memory_regions_for_serialization| {
let mut os_ipc_shared_memory_regions_for_serialization =
os_ipc_shared_memory_regions_for_serialization.borrow_mut();
let index = os_ipc_shared_memory_regions_for_serialization.len();
os_ipc_shared_memory_regions_for_serialization.push(self.os_shared_memory.clone());
index
},
);
index.serialize(serializer)
if let Some(os_shared_memory) = &self.os_shared_memory {
let index = OS_IPC_SHARED_MEMORY_REGIONS_FOR_SERIALIZATION.with(
|os_ipc_shared_memory_regions_for_serialization| {
let mut os_ipc_shared_memory_regions_for_serialization =
os_ipc_shared_memory_regions_for_serialization.borrow_mut();
let index = os_ipc_shared_memory_regions_for_serialization.len();
os_ipc_shared_memory_regions_for_serialization.push(os_shared_memory.clone());
index
},
);
debug_assert!(index < usize::MAX);
index
} else {
usize::MAX
}
.serialize(serializer)
}
}

impl IpcSharedMemory {
const fn empty() -> Self {
Self {
os_shared_memory: None,
}
}

/// Create shared memory initialized with the bytes provided.
pub fn from_bytes(bytes: &[u8]) -> IpcSharedMemory {
IpcSharedMemory {
os_shared_memory: OsIpcSharedMemory::from_bytes(bytes),
if bytes.is_empty() {
IpcSharedMemory::empty()
} else {
IpcSharedMemory {
os_shared_memory: Some(OsIpcSharedMemory::from_bytes(bytes)),
}
Comment on lines 630 to +636
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be made even simpler by having this return Option<IpcSharedMemory>, much like ptr::NonNull? Then calling code would explicitly need to handle when the shared memory is empty. It feels like it should be an error to try to create an empty shared memory segment -- as it's kinda useless.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would made uses more verbose (most of the time there would be Some value so a lot of None values would be unreachable) and I feel this would complicate things for consumers. There is also no way in rust to tell that slice is at least length of 1, so consumers would still need to handle len=0 cases, even if they get Some. Also this would be breaking change and I tried to avoided that.

Empty buffer also has meaning (receiver was woken with empty message).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that this API needs to change anyway. Creating a shared memory segment can fail (see the manpage for shm_open for instance). ipc-channels is pretending like it cannot fail and it will panic if a failure does occur. When creating IPC channels, this reality is acknowledged, because channel() returns a Result. Not doing the same here feels like a bit of a lie. Many programs will want to handle failure gracefully.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but I think better error handling should be done as in separate PR. This PR just makes current API works as expected.

}
}

/// Create a chunk of shared memory that is filled with the byte
/// provided.
pub fn from_byte(byte: u8, length: usize) -> IpcSharedMemory {
IpcSharedMemory {
os_shared_memory: OsIpcSharedMemory::from_byte(byte, length),
if length == 0 {
IpcSharedMemory::empty()
} else {
IpcSharedMemory {
os_shared_memory: Some(OsIpcSharedMemory::from_byte(byte, length)),
}
}
}
}
Expand Down
20 changes: 20 additions & 0 deletions src/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,26 @@ fn shared_memory() {
.all(|byte| *byte == 0xba));
}

#[test]
fn shared_memory_slice() {
let (tx, rx) = ipc::channel().unwrap();
// test byte of size 0
let shared_memory = IpcSharedMemory::from_byte(42, 0);
tx.send(shared_memory.clone()).unwrap();
let received_shared_memory = rx.recv().unwrap();
assert_eq!(*received_shared_memory, *shared_memory);
// test empty slice
let shared_memory = IpcSharedMemory::from_bytes(&[]);
tx.send(shared_memory.clone()).unwrap();
let received_shared_memory = rx.recv().unwrap();
assert_eq!(*received_shared_memory, *shared_memory);
// test non-empty slice
let shared_memory = IpcSharedMemory::from_bytes(&[4, 2, 42]);
tx.send(shared_memory.clone()).unwrap();
let received_shared_memory = rx.recv().unwrap();
assert_eq!(*received_shared_memory, *shared_memory);
}

#[test]
#[cfg(any(
not(target_os = "windows"),
Expand Down
Loading