A few notes

[DzenanJupic]

Just saw this on Reddit, and I think I have to leave a few notes on the code.

I don't want to demotivate you and can offer to send in a few PRs or help abstract unsafe stuff into concise modules. But since this is on crates.io, these issues should definitely be publicly addressed.

I didn't get through the whole code within the few minutes I had, and I also cannot even remotely prove that the concept is sound or unsound yet, but these are a few of my first findings (not ordered in any particular order):

• That looks like it could reactivate terminated Signals and also result in two instances with an unlocked state. Also, a SAFETY comment would be nice in places like that, since the reader has no clue why this is supposed to be sound. https://github.com/fereidani/kanal/blob/d8f816a9c9255ccebce3cbaa9fd7cf6f7c3cd41e/src/signal.rs#L193-L194

• In a lot of places, there are separate branches for ZSTs, that make the code harder to read, but end up being ignored by the compiler anyways (godbolt). https://github.com/fereidani/kanal/blob/d8f816a9c9255ccebce3cbaa9fd7cf6f7c3cd41e/src/signal.rs#L76-L83

• Also, in some cases, these branches also change the code's behaviour, since ZSTs are dropped at a different point in time. Since the Drop implementation can have side effects (i.e. modify thread locals) that's not quite right (playground). https://github.com/fereidani/kanal/blob/d8f816a9c9255ccebce3cbaa9fd7cf6f7c3cd41e/src/signal.rs#L116-L122

• Mutex<Cell<T>> seems odd. Not sure what the intention of that is. https://github.com/fereidani/kanal/blob/d8f816a9c9255ccebce3cbaa9fd7cf6f7c3cd41e/src/signal.rs#L37

• SendSignal has unconstraint Send + Sync impls. Using this (Not used currently as far as I can see), it's possible to send !Send values to other threads. That can lead to data races, double free, and use after free. https://github.com/fereidani/kanal/blob/d8f816a9c9255ccebce3cbaa9fd7cf6f7c3cd41e/src/signal.rs#L165-L166

• There are a lot of pub items, that are not publicly reachable. This makes the code a bit harder to read since it's never clear if that's a public-facing API. And with some of the functions is crate, you really don't want them to be pub. pub(crate) could help here.

• acquire_internal(&self.internal) vs. self.internal.lock() seems to decrease readability while not really saving characters. https://github.com/fereidani/kanal/blob/d8f816a9c9255ccebce3cbaa9fd7cf6f7c3cd41e/src/internal.rs#L12-L14

• forget is free, so there's no need to 'optimize' it away (godbolt) https://github.com/fereidani/kanal/blob/d8f816a9c9255ccebce3cbaa9fd7cf6f7c3cd41e/src/lib.rs#L198-L200

[fereidani]

Thank you for your explanations, I agree that unsafe code should get more organized, Some note that I have is I'm using acquire_internal to test multiple locks that are using different interfaces. That function helps me test and benchmark multiple types of mutexes through development. About Only one Receiver will ever receive the data sent it's samething to Golang, it's not a broadcast channel. every message should deliver only once. I'll be happy and thankful if you review the code more and send us PR.

[DzenanJupic]

it's not a broadcast channel. every message should deliver only once

In that case, it might make sense to make Receiver not be Clone

[adrian-budau]

You do want receiver to be Clone so you can have multiple receiver threads, for example worker threads doing some cpu intensive things.

Most other implementations DO NOT actually send to all receivers, i.e.: https://docs.rs/crossbeam/latest/crossbeam/channel/struct.Sender.html does not require T: Clone. And the documentation itself says:

#Note that cloning only creates a new handle to the same sending or receiving side. It does not create a separate stream of messages in any way:

use crossbeam_channel::unbounded;

let (s1, r1) = unbounded();
let (s2, r2) = (s1.clone(), r1.clone());
let (s3, r3) = (s2.clone(), r2.clone());

s1.send(10).unwrap();
s2.send(20).unwrap();
s3.send(30).unwrap();

assert_eq!(r3.recv(), Ok(10));
assert_eq!(r1.recv(), Ok(20));
assert_eq!(r2.recv(), Ok(30));```

[DzenanJupic]

Interesting didn't know that, thanks.

[DzenanJupic]

Just for reference: #7

[fereidani]

Safety comments added by last commit to the project, I'm closing the issue. feel free to reopen it if you find any unresolved problem.