zeromq / netmq

A 100% native C# implementation of ZeroMQ for .NET
Other
2.97k stars 746 forks source link

LazyPirateClient never terminates #130

Closed edward-ellis closed 10 years ago

edward-ellis commented 10 years ago

While trying to learn about the lazy pirate sample I tried terminating the server to see if the client would exit as advertized. It doesn't exit. When trying to dispose of the context the Ctx.Terminate makes a call:

cmd = m_termMailbox.Recv(-1);

The mailbox makes the call:

bool rc = m_signaler.WaitEvent(timeout);

The signaller makes a call:

return m_r.Poll(timeout * 1000, SelectMode.SelectRead);

The result is that we've called Poll on the socket with a timeout of -1000 milliseconds. I don't think that is an appropriate argument, and so I'm assuming that one of the containing classes was supposed to trap the timeout == -1 and behave differently. However, since none of these methods describe their contract, I'm not sure where that is supposed to happen.

edward-ellis commented 10 years ago

SocketBase has a comment about timeout is -1 and changes its behavior, but it just passes the negative timeout right on through to the Mailbox. Since neither the Mailbox nor the Signaler is handling negative timeouts, I'm going to assume the fault lies in one of them. However, it could be that the Ctx is waiting for the reaper thread to close the sockets and there is something amiss there.

edward-ellis commented 10 years ago

Well I am learning a lot. It turns out that the example is flawed in both C# and Python, so I'm going to go back to the book author before I do anything here.

reiroldan commented 10 years ago

Did you close the socket before closing the context?

Could you please provide the code you are using?

edward-ellis commented 10 years ago

I'm using Samples\Pirate Pattern\Lazy Pirate\LazyPirate.Client\Program.cs. I believe this is a fairly faithful reproduction of the sample in the book, but converted to use NetMQ. The problem is that the sample in the book is wrong because it doesn't close the socket. I've contacted the author who has the book in GitHub repo imatix/zguide. He has suggested that I submit a pull request with the appropriate changes. I found this bug while using the sample as a pattern for code at work. However, changing his book and the NetMQ samples will have to be a spare time activity. I intend to eventually submit a pull request for the book and then a pull request for NetMQ. In the mean time I wanted a bug report to assist others that run into the same problem with this sample. Would you prefer that I change the NetMQ sample before I change the book? The C# solution is much simpler than updating the book.

tobi-tobsen commented 10 years ago

Are you referring to this line which should be wrapped in a using statement (or at least it should be disposed later on I guess....) in order to dispose the socket before the context gets disposed?

somdoron commented 10 years ago

@tobi-tobsen or @d3j409 can we close the issue?

tobi-tobsen commented 10 years ago

I think from the NetMQ perspecitve this can be closed. That being said, @d3j409 is right, the samples in the zguide are still wrong (should be a separate issue there) and the sample code in the guide should be updated to NetMQ instead of clrzmq.