zeromq / netmq

A 100% native C# implementation of ZeroMQ for .NET

Other

2.97k stars 746 forks source link

Received messages via push/pull are empty #25

Closed tobi-tobsen closed 12 years ago

tobi-tobsen commented 12 years ago

Bug

Received messages via push/pull are empty. Both the Size property as well as the length of the Data byte[] property are 0.

Steps to reproduce

get the performance test from #24
compile the solution
run the following: local_thr.exe tcp://127.0.0.1:4711 300 1000
run the following: remote_thr.exe tcp://127.0.0.1:4711 300 1000

somdoron commented 12 years ago

i will fix it, it better to fix this to have same behavior as c++.

tobi-tobsen commented 12 years ago

I tried removing the the call to close() and it works now. #27

I am not sure whats the best behaviour/API is:

If there is a Close() should one call it after the message has been send?
A Message being IDisposable has the same semantics I guess
socket.Send(Msg) seems to fiddle around with the message after the call. In order to work correctly with Close() a copy might have to be made somewhere. I don't think that is a good thing to do either. I am currently not sure how the c++ implementation handles that.

Maybe garbage collection is sufficient and there is no need for an explicit call to close?

somdoron commented 12 years ago

c++ is copy the msg class but the buffer is not copied. we may do the same

somdoron commented 12 years ago

i just added a new pull request with performance feature from original zeromq.

the throughput performance is looking good compare to the original.

i hope latency check is coming soon

tobi-tobsen commented 12 years ago

From inspection of your pullrequest, the issue seems to be fixed and it seems that a call to Close is still not necessary but doesn't hurt either since it is just setting the message type to VSM (very small message) (https://github.com/zeromq/netmq/blob/master/src/NetMQ/zmq/Msg.cs#L273).

If you want so, I'll add the latency performance test after work.

somdoron commented 12 years ago

that will be great. thanks

somdoron commented 12 years ago

fixed in last merge pull