Support empty IpcSharedMemory #335
Conversation
| 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)), | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Indeed, but I think better error handling should be done as in separate PR. This PR just makes current API works as expected.
[](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [ipc-channel](https://github.com/servo/ipc-channel) | dependencies | patch | `0.18.0` -> `0.18.2` | --- ### Release Notes <details> <summary>servo/ipc-channel (ipc-channel)</summary> ### [`v0.18.2`](https://github.com/servo/ipc-channel/releases/tag/v0.18.2) [Compare Source](https://github.com/servo/ipc-channel/compare/v0.18.1...v0.18.2) Changes: - Upgraded to version `1.0` of `mio` ### [`v0.18.1`](https://github.com/servo/ipc-channel/releases/tag/v0.18.1) [Compare Source](https://github.com/servo/ipc-channel/compare/v0.18.0...v0.18.1) Includes [https://github.com/servo/ipc-channel/pull/335](https://github.com/servo/ipc-channel/pull/335) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 3pm on Wednesday" in timezone America/New_York, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- Release Notes: - N/A <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
#334 made more simple.
Fixes #268.