Skip to content

Improve Event Broadcaster robustness and feedback on error#265

Merged
aacuevas merged 4 commits intoopen-ephys:developmentfrom
tne-lab:event-broadcaster-robust
Oct 10, 2018
Merged

Improve Event Broadcaster robustness and feedback on error#265
aacuevas merged 4 commits intoopen-ephys:developmentfrom
tne-lab:event-broadcaster-robust

Conversation

@ethanbb
Copy link
Copy Markdown
Contributor

@ethanbb ethanbb commented Oct 8, 2018

This incorporates a few changes to try to make the Event Broadcaster more predictable and less frustrating to use:

  • Previously, the socket would silently fail to bind to the port every other time you clicked "Restart connection" (at least on my machine), due to the old socket not actually closing and freeing the port immediately. This also caused race conditions when loading a signal chain. The issue is discussed here (for the Node.js version): socket.close() is not really synchronous but there is no event to notify when it is really closed JustinTulloss/zeromq.node#214. To solve, as suggested there, I added a call to zmq_unbind which frees the port immediately. The old socket then only gets closed if the new one is bound successfully; otherwise, it gets rebound to the original port.

  • When loading a signal chain, previously the editor would not be updated to show the loaded port. Fixed this by adding a method in the editor to update the displayed port and calling this whenever the port is set.

  • If 5557 was already in use, a new Event Broadcaster would default to port 0 (invalid), which is not ideal. Instead, changed the constructor to keep looking for a free port if it receives the EADDRINUSE error from setListeningPort.

  • Made the editor report errors in the status bar if they occur when changing the port or restarting the connection.

I briefly looked at the Network Events plugin as well, but it looks like the code there is substantially different even though the editor's appearance is similar to Event Broadcaster. So I didn't want to get involved with that too given that this is already a medium-sized PR.

@ethanbb
Copy link
Copy Markdown
Contributor Author

ethanbb commented Oct 10, 2018

Added a solution to the actual problem that was causing #264 (...causing me to think it was necessary). It uses a reference counted context object to destroy the context when the last event broadcaster is deleted rather than when the program exits. Sorry if it's too big of a change/too complicated, but it seems to work.

@aacuevas
Copy link
Copy Markdown
Collaborator

I was just writing about that.
Adding a reference count is what I was thinking about #264 so perfect.

Thanks!

@aacuevas aacuevas merged commit 11bc708 into open-ephys:development Oct 10, 2018
Copy link
Copy Markdown
Contributor Author

@ethanbb ethanbb left a comment

Choose a reason for hiding this comment

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

@aacuevas Thanks for the quick merge! My fault for pushing to an open PR, but I noticed a couple things after it was merged that could be improved - noted below.

ScopedLock lock(sharedContextLock);
sharedContext = nullptr;
#ifdef ZEROMQ
zmq_ctx_destroy(context);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe would be better to release the lock before calling this (by putting the 2 lines above in their own block). Theoretically all the sockets should be gone if this is being called, but it doesn't hurt and since this call blocks if there are still any alive sockets, there could be some potential for a deadlock.

#ifdef ZEROMQ
jassert(context != nullptr);
return zmq_socket(context, ZMQ_PUB);
#endif
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Realized soon after the last commit that there should be a return statement for the case that ZEROMQ isn't defined (would just return nullptr). I have a feeling this would produce at least a warning on Linux.

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