Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bad assertion #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix bad assertion #6

wants to merge 2 commits into from

Conversation

zh217
Copy link

@zh217 zh217 commented Sep 1, 2014

This assertion will fail under legitimate circumstances.

Consider the following sequence of events:

  1. A pull socket is registered
  2. Async loop has the socket in the pairings map
  3. ZMQ loop has the socket in the socks map
  4. The in channel of the socket is closed, and at the same time a message is received from the ZMQ socket
  5. Async loop will dissoc the socket from the pairings map
  6. ZMQ loop will relay the message to the async loop
  7. Async loop will look up the socket from the pairings map, but the socket in question has already been dissoc'ed
  8. An exception is thrown, after which nothing works anymore.

@lynaghk
Copy link
Owner

lynaghk commented Sep 2, 2014

My concern with your fix is that it will be very difficult for someone to debug the situation where they accidentally forget to provide an out channel.

Did you run into the situation you outlined above, or is it a theoretical concern?

@zh217
Copy link
Author

zh217 commented Sep 3, 2014

Yes I run into the situation. It happens about every six hours or so in a system where constantly ~100 sockets are open.

A better place to put the assertion would be when creating the socket.

@lynaghk
Copy link
Owner

lynaghk commented Sep 3, 2014

Ah, I had no idea anyone was using this library = )

Putting assertions for in/out channels where the socket is created sounds like a good idea; it just needs to match up with the ZeroMQ socket types (since some of them don't always take both).

Would be happy to accept a pull request for that.

@zh217
Copy link
Author

zh217 commented Sep 3, 2014

Done.

Omitting in channel seems like a bad idea to me, because then you have no way of shutting down the socket. Maybe it is legitimate for systems that don't manage sockets dynamically.

@lynaghk
Copy link
Owner

lynaghk commented Sep 3, 2014

Awesome, looks good to me. Did you also want to remove the original assertion you opened the issue about?

@zh217
Copy link
Author

zh217 commented Sep 3, 2014

Yes it is removed. This pull request contains two commits, the first one does that, and the second one adds the new assertions.

@lynaghk
Copy link
Owner

lynaghk commented Sep 4, 2014

Oh, sorry. I didn't notice that. Can you please squash these into a single commit with descriptive message (e.g., "Check for in/out chans when registering socket rather than within internal machinery")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants