Send different pointer structs through Channel and receive the same value

Help & Support
1

Question

Hi everyone, I am using Crystal Forum for the first time and I am having some tough problems with Crystal.

  • i. Send different pointer structs through Channel and receive the same value (Maybe where am I doing wrong?).
  • ii. It seems that in Golang there is no class (only struct), in Crystal Mutex, RWLock, SpinLock seem to be all classes?
  • iii. Even using (pointerof(element).as(UInt8).to_slice(sizeof(typeof(element))).dup), (staged_received_pointer.to_unsafe.as(QueueOutboundElement*).value), Mutex in struct will trigger Invalid Memory Access.
  • iv. Crystal and Golang I’m just a rookie, if I’m wrong, please enlighten me. :slight_smile:

Crystal

@[Packed]
struct QueueOutboundElement
  property test : Int32
 
  def initialize(@test : Int32)
  end
end
 
staged = Channel(QueueOutboundElement*).new capacity: 128_i32
 
spawn do
  128_i32.times do
    staged_received_pointer = staged.receive
    STDOUT.puts [staged_received_pointer.value]
  end
end
 
128_i32.times do
  element = QueueOutboundElement.new Random.new.rand type: Int32
  staged.send value: pointerof(element)
end
 
loop do
  sleep 1_i32.seconds
end

Golang


peer.queue.outbound = newAutodrainingOutboundQueue(device)
peer.queue.staged = make(chan *QueueOutboundElement, QueueStagedSize)
device.queue.encryption = newOutboundQueue()


func (device *Device) RoutineReadFromTUN() {
	...
	var elem *QueueOutboundElement
	...

	for {
		elem = device.NewOutboundElement()
		...
		if peer.isRunning.Load() {
			peer.StagePacket(elem)
			elem = nil
			peer.SendStagedPackets()
		}
		...
	}
}

func (peer *Peer) StagePacket(elem *QueueOutboundElement) {
	for {
		select {
		case peer.queue.staged <- elem: // Put
			return
		default:
		}
		select {
		case tooOld := <-peer.queue.staged:
			peer.device.PutMessageBuffer(tooOld.buffer)
			peer.device.PutOutboundElement(tooOld)
		default:
		}
	}
}

func (peer *Peer) SendStagedPackets() {
top:
	...

	for {
		select {
		case elem := <-peer.queue.staged: // Get
			elem.peer = peer
			elem.nonce = keypair.sendNonce.Add(1) - 1

			if elem.nonce >= RejectAfterMessages {
				keypair.sendNonce.Store(RejectAfterMessages)
				peer.StagePacket(elem) // XXX: Out of order, but we can't front-load go chans
				...
				goto top
			}

			elem.keypair = keypair
			elem.Lock()

			if peer.isRunning.Load() {
				peer.queue.outbound.c <- elem
				peer.device.queue.encryption.c <- elem
			} else {
				peer.device.PutMessageBuffer(elem.buffer)
				peer.device.PutOutboundElement(elem)
			}
		default:
			return
		}
	}
}

IRC

FromGitter 03:05:46
<Blacksmoke16> you kinda me 😅. Maybe a forum thread would be a better way

@Blacksmoke16 yes, but didn't find a solution about this issue from the forum thread, so I came to IRC, I wanted to use Crystal to do something like WireGuard, but I ran into this problem and still haven't solved it after 2 days of research \O/  =$

<Blacksmoke16> right, make a new thread about your issue :P

@Blacksmoke16 ok thanks, you are a great person, I will go to the forum to create an issue later. (y)
2

You’re taking a pointer to a local variable which lives on the stack and is overwritten in every iteration of the loop. Stack pointers are very unsafe. You should only use them if you know what you’re doing, i.e. you can guarantee that the stack location is still valid when the pointer is dereferenced.
That’s not the case in this example.

1 Like
3

Suggestion: change QueueOutboundElement to be a class. Then don’t use pointers. A class is already a pointer-like type (it’s a reference type.)

In Go if you take a pointer/reference to something, Go is smart enough to know whether it should move that value to the heap. Crystal doesn’t work that way.

Some more suggestions:

  • Don’t use struct unless performance benchmarks show that you need it
  • Never use pointerof
  • Never use pointers

The last two are really low level things that are used to implement the basic types in the standard library, and for interfacing with C. But in general you will never use them.

4 Likes
4

Yeah, the easiest option is to allocate the objects on the heap with a class type. Or you could perhaps just pass the struct directly through the channel instead of a pointer.

1 Like
5

Finally (Update)

  • After hours of re-research, Finally found the solution: use Pointer(T).malloc + Pointer.copy_from.
  • Although manipulating pointers can be dangerous, performance should be consistent with Golang.
  • @straight-shoota, @asterite you guys are Crystal-lang legends, thanks for your answers, I get it!

Crystal

struct QueueOutboundElement
  property test : Int32
  getter mutex : Mutex

  def initialize(@test : Int32)
    @mutex = Mutex.new :unchecked
  end

  def lock : Nil
    @mutex.lock
  end

  def unlock : Nil
    @mutex.unlock
  end
end

module StructCloner(T)
  def self.clone(source : T) : T*
    new_pointer = Pointer(T).malloc size: sizeof(typeof(T))
    new_pointer.copy_from source: pointerof(source), count: sizeof(typeof(T))

    new_pointer
  end
end

staged = Channel(QueueOutboundElement*).new capacity: 128_i32
outbound = Channel(QueueOutboundElement*).new capacity: 128_i32
encryption = Channel(QueueOutboundElement*).new capacity: 128_i32

spawn do
  128_i32.times do
    received_outbound = outbound.receive
    received_outbound.value.lock

    received_outbound.value
  end
end

spawn do
  128_i32.times do
    received_encryption = encryption.receive
    received_encryption.value.test = Random.new.rand(type: UInt8).to_i32

    sleep 0.01_f32.seconds
    received_encryption.value.unlock
  end
end

spawn do
  128_i32.times do
    staged_received_pointer = staged.receive
    staged_received_pointer.value.lock

    outbound.send value: staged_received_pointer
    encryption.send value: staged_received_pointer
  end
end

128_i32.times do
  element = QueueOutboundElement.new Random.new.rand type: Int32
  cloned_element = StructCloner(QueueOutboundElement).clone source: element
  cloned_element.value

  staged.send value: cloned_element
end

loop do
  sleep 1_i32.seconds
end
6

There are a couple of problems with your code.

Pointer(T).malloc size: sizeof(typeof(T)) is doubly wrong. It works but it over-allocates by 4 times the necessary amount.

I understand that with sizeof(typeof(T)) you probably tried to tell Pointer.malloc the size of the object you want to allocate. But that’s already handled by Pointer(T). And then T is a class type, so typeof(T) is always Class, meaning sizeof(typeof(T)) is always 4.

Pointer(T).malloc(size) allocates memory for size time instances of T. Your solution needs only one instance, so the argument should be 1 (or omitted because that’s the default value).

Same applies to the count argument of Pointer#copy_from. You’re telling it to read 4 instances of T from the pointer of source which actually reads unallocated memory ahead of the stack. Again, count should be 1.
And for what you’re doing, you don’t need copy_from. Just Pointer#value= would do. You can even incoporate the value into Pointer#malloc:
Instead of that StructCloner thing, you get the same result with `Pointer(QueueOutboundElement).malloc(1, element).

I’d recommend you read the API docs more carefully because the behaviour of these methods is really all explained there and it is pretty obvious that you’re using it wrongly.

Finally, I don’t see the point why you’re going through all the trouble with manually pushing a struct to the heap. If you’re gonna allocate on the heap anyway, you could just use a class type in the first place. That’s much easier to handle because the runtime takes care of allocating and dereferencing. You’re just doing a lot of extra work to get basically the same behaviour as you would get with a class.