memory not freed

[je-so]

1. blocking_pipe_read { ... void* msg_ptr = malloc(sizeof(void_)) ... }
   Where is the corresponding free?
   void_ msg;
   chan_recv(chan, &msg);
   printf("%s\n", msg);
   // missing free(msg)
   chan_recv(chan, &msg);
   printf("%s\n", msg);
   // missing free(msg)
2. Why do you write void* into a pipe and also synchronize with pthread_cond_wait?
   It is much simpler to synchronize with pthread_cond_wait and write a pointer into a data field of blocking_pipe_t*. You do not need any pipe in this case.
3. How do make sure that the content where the pointer points to is valid?
   What about the following ?
   void* ping() {
   char localstr[] = { "ping" };
   chan_send(chan, localstr);
   return NULL;
   }
   This causes undefined behaviour. Your synchronization in blocking_pipe_t synchronizes
   only the pointer transfer but does not make sure that the receiving end has processed
   the data the pointer is pointing to !!
4. Error: not unlocking mutex
   unbuffered_chan_recv(chan_t* chan, void** data)
   {
   pthread_mutex_lock(&chan->r_mu);
   if (chan_is_closed(chan))
   {
   // missing _unlock !!
   errno = EPIPE;
   return -1;
   }

(No more time to check for more errors :-))

[tylertreat]

@je-so I added numbers to your comment so I could address each item.

1. I think the msg pointer malloc is actually unnecessary altogether, so I removed it.
2. I might need to revisit this code, but I think there would be a potential race condition between the reader and writer. We need to ensure the reader waits until the data is available, so the blocking on the read end of the pipe allows this.
3. I'm still not entirely sure what a good solution to this would be.
4. Good catch. :)

[je-so]

1. OK.
2. Nope. Setting pipe->data = data first and then signalling the condition and then unlocking the mutex
   ensures, the writer synchronizes with the reader. The reader waits for the condition, acquires the mutex which synchronizes memory (memory fence), so pipe->data contains the value written by the writer.
3. The first solution I'd come up with is to use a garbage collected language :-)
   But in C you have the writer call another function like chan_waituntildataprocessed before the
   data goes into an undefined/freed state. The reader should call something like chan_signaldataprocessed()

[tylertreat]

The other reason the pipe is useful is when a channel is closed. In the event that a reader is blocked waiting, closing a channel needs to unblock the reader and have it return -1. This could probably be implemented without the pipe, but it would require the chan to signal the pipe's cond and the pipe reader to check if the chan has been closed. One option might be to add a field on the blocking pipe struct closed that the chan sets, but this starts to complicate blocking pipe a little more. It would also require thread safety around the closed field.

I suppose writing/reading from a pipe is significantly slower than simply assigning the void pointer though.

[je-so]

Yep.

Writer: Uses a simple queue which can store 16 void* and calls put_voidptrm which searches the next free entry waiting on condition_notfull if queue is full.

Reader: Calls get_voidptr, reading from head of queue and waiting on condition_notempty if queue is empty.

Reading signals condition_notfull and writing signals condition_notempty.

Closing the chan flushes the queue (removing unprocessed content).
Closing also acquires the mutex before flushing, signals both conditions and sets some flag
which could be as simple as setting the queue storage to null.

If you have many readers and writers and the channel could be closed and freed any time
it is necessary to wait during the close operation until all other threads have been notified.
A thread which calls a read or write op must check for a special error case (EPIPE) which
means the pipe (internal queue) is closed and never ever call any function on this channel
in the future.

[je-so]

Point 3: How do you make sure that the content where the pointer points to is valid?

I thought about the problem and implemented an experimental message queue
(I want to implement one for my database project where zero-copy is paramount)

See https://github.com/je-so/iqueue/blob/master/README.md for an example how the
server thread signals message processed to the client.

What do you think?

[tylertreat]

That looks like a reasonable solution. The problem with chan is I'd like to maintain the semantics of Go channels as much as possible. I think this would break from those semantics.

A way to send/recv buffers safely was implemented using memcpy (https://github.com/tylertreat/chan/blob/master/src/chan.c#L591). Zero-copy is really nice, but I think this matches the semantics of Go more.

[je-so]

I think instead of calling chan_send_buf and chan_recv_buf it is much faster to send a pointer to
a malloced buffer directly and free the buffer on the receiving end -- which would save 2 memcpy and one malloc and free operation.

But I can see the point of being compatible with Go channels which employ value semantics.

Then wouldn't it be faster to store the whole value in chan instead of a pointer to a malloced value?

If you'd provide an elemsize parameter in the init function and change recv and send to transfer elemsize bytes instead of a single pointer chan would reflect the value semantics of GO even more.
It is not zero-copy but much faster than calling malloc/free for every single element.

You also have to provide the type (== elemsize) parameter in the declaration of a Go channel :-)

[tylertreat]

Yes, there has been some discussion on "typed" channels (#7), but I agree that specifying an elemsize in init would probably be a better solution. It would indeed closer match the semantics of Go since you have to specify a type when creating a channel.

[je-so]

If have experimented with iqueue and added a type safe feature which is implemented in macro iqueue_DECLARE (see https://github.com/je-so/iqueue/blob/master/include/iqueue.h at the bottom). You could use the same technique to make chan type safe (after you have added support for elemsize of course :-)).

(example 3 shows how to use the macro).

[tylertreat]

Yeah, that looks really nice!