Skip to content

Add getter for socket closed state - #181

Merged
rgbkrk merged 2 commits into
zeromq:masterfrom
rolftimmermans:state
Sep 19, 2017
Merged

Add getter for socket closed state#181
rgbkrk merged 2 commits into
zeromq:masterfrom
rolftimmermans:state

Conversation

@rolftimmermans

Copy link
Copy Markdown
Member

In my code I have found it useful to be able to determine if a socket is closed. This is information is already kept in a state variable in the C++ bindings, but not exposed as public API.

This PR proposes to add a socket.closed getter, which returns a boolean.

(An alternative implementation would be a socket.state getter which returns the socket's internal state. Which state values are possible would not be immediately obvious to me, so I'm proposing this boolean getter instead.)

The background of being able to check if a socket is closed is that some monitor events (and other events in my application) can occur slightly out of order due to timing issues native to how Node works. As such it is nice to avoid calling socket.send() or something similar if the socket was already closed with socket.close() as a result of another event. Of course I could keep this state in my application, but that seems needlessly complex given that the bindings already keep track of the state of the socket!

@rolftimmermans rolftimmermans changed the title Add accessor for socket closed state Add getter for socket closed state Sep 2, 2017

@lgeiger lgeiger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this. 👍

I agree it's a good idea to have an easy way to see if a socket is closed or not.

@rgbkrk
rgbkrk merged commit ba6b1c3 into zeromq:master Sep 19, 2017
@ronkorving

ronkorving commented Sep 20, 2017

Copy link
Copy Markdown

We need to really improve the docs. I love little additions like this, but there's no way for users to discover them.

For the record, and for inspiration, I do have a WIP PR (that has been unfinished for ages, I need to get back to it at some point) with a much more completed ReadMe. By all means have a look and let me know what you think

@rgbkrk

rgbkrk commented Sep 20, 2017

Copy link
Copy Markdown
Member

We should definitely include these changes in the release notes.

By all means have a look and let me know what you think

I think we should take small parts out of your PR and bring them in as individual PRs. Making the module export no longer the binding is a simple one that we could bring in before we make a major release (master is currently slated for a major release after #182). I don't want to hold it up too long though.

@ronkorving

Copy link
Copy Markdown

Oh sure, I was really just aiming at the ReadMe for now :) Would love to get feedback on its form.

@roccomuso

Copy link
Copy Markdown

The getter works when socket.close() is called. Why event close is never fired though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants