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

if an event is an error, pass the exception object #3

Closed
wants to merge 1 commit into from

Conversation

cxreg
Copy link

@cxreg cxreg commented Nov 16, 2016

Upstream PR JustinTulloss#578 has not been responded to at this time. I propose to merge this to our fork for now.

@pmuellr
Copy link

pmuellr commented Nov 16, 2016

Still a little bothered by having a separate upstream PR, esp since we don't know the state of it.

I believe the last changes to storage to produce better error messages for already bound zmq ports will be a good enough test to make sure nothings broken between node/npm-zmq and nsolid - make sure you run the storage integration tests w/nsolid to make sure nothings b0rken ...

Other than that, LGTM, but not really familiar with the NAN usage in the natives ...

@cxreg
Copy link
Author

cxreg commented Nov 16, 2016

The NAN usage was copied from neighboring code, so it should be ok

I'll ping upstream one more time...

case "bind_error":
case "accept_error":
case "close_error":
ex = self._zmq.getException();

Choose a reason for hiding this comment

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

probably wanna put a break here to prevent fall through if more cases are introduced

@thlorenz
Copy link

In general LGTM

@cxreg
Copy link
Author

cxreg commented Nov 21, 2016

Upstream accepted a modified version of this but it's not merged yet. Keeping this open for a few more days to see how that shakes out and when it lands

@cxreg
Copy link
Author

cxreg commented Nov 21, 2016

Upstream fix merged and landed, closing this

@cxreg cxreg closed this Nov 21, 2016
This pull request was closed.
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.

3 participants