Clean up and improve Network Events#276
Conversation
|
Thanks, this looks awesome! We will test it out and get it merged soon. |
|
Indeed. Seems like a much needed cleanup! |
ethanbb
left a comment
ethanbb
left a comment
There was a problem hiding this comment.
Thanks for taking a look! I just had a couple more thoughts, see below.
| #else | ||
| usleep (300 * 1000); | ||
| #endif | ||
| ScopedPointer<Responder> newResponder = Responder::makeResponder(port); |
There was a problem hiding this comment.
We could make the socket within the thread instead (discarding it without destroying the old one if it fails), and just set urlport here (which we could make atomic) to indicate a port change. Then we wouldn't need nextResponder or the lock. I did it this way so that the caller would immediately know whether the change was successful, but that's actually not a big deal since the thread can just update the port on the editor if the switch fails.
| responder = nullptr; | ||
| urlport = 5556; | ||
| threadRunning = false; | ||
| if (!setNewListeningPort(5556)) |
There was a problem hiding this comment.
If we changed setNewListeningPort to not create the socket and return feedback right away, we couldn't do this, so maybe it would be better to just choose the port automatically by inputting 0. Of course loading a signal chain would still switch the port to the saved value, if possible.
| unsigned char* buffer = new unsigned char[MAX_MESSAGE_LENGTH]; | ||
| int result = -1; | ||
| ScopedPointer<Responder> responder; | ||
| char buffer[MAX_MESSAGE_LENGTH]; |
There was a problem hiding this comment.
This should probably be a HeapBlock rather than an array on the stack, actually. I was thinking I could use a plain array since the size never changes, but I wasn't considering the general rule that large arrays should be on the heap.
|
I'm going to go ahead and make another commit with these changes, unless anyone disagrees or wants to discuss them further. |
|
Works beautifully, thanks again for this! |
|
You're welcome! I'm glad you didn't have any issues with it. |
3 participants
I started out just wanting to fix the port on the editor not updating when loading a signal chain. I found some more issues with the plugin though, so I decided to try fixing them. Sorry for the huge diffs... However, I think you'll find it resolves most of the instability issues that Network Events has suffered from. Also fixes #174.
Overview of changes:
Uses RAII wrappers for the context and socket to avoid leaking memory. In particular, similar to the latest Event Broadcaster, the context is shared between instances via a raw pointer but "owned" jointly by all sockets using reference-counted pointers, so that it is destroyed after all instances of the plugin are destroyed. This avoids a crash with ZeroMQ on Windows that happens when a context is destroyed during unloading of a DLL.
Avoids destroying the sockets in the main thread (or interacting with them at all except for creating them and passing them to the thread using a lock). ZMQ sockets are not thread-safe, and the call to
zmq_closeon restart, port change, and shutdown was causing some crashes - possibly the cause of Network Events causes crashes when restarting connection #174.Doesn't block forever on
zmq_recv; instead has a timeout of 100 ms. This means possibly some added latency while the thread checks whether it should exit, change port or restart between calls, but these checks should be extremely fast, and the upside is that we don't need to destroy the context each time we need to take one of these actions. The context should typically persist for the lifetime of the program, and creating a new one has a high overhead. As a consequence:Updates the port label to always show:
<no cxn>if not connected<no zeromq>if it was not compiled with ZeroMQAllows inputting a "*" as the port to use any available port. The label then updates to show what port was actually selected.
If a port change fails and a socket was connected on the previous port, keeps using that port and socket.
Sends the status message when receiving an event from the thread rather than the process function, so that it shows up even if acquisition isn't running.
Simplifies
StringTSsignificantly - it doesn't really need to be more complicated than aStringcoupled with a timestamp. In fact, we could usestd::pairinstead, but having names for the members is nice.Deletes the code related to simulations, since it seemed to be unused. This is all in the last commit since I wasn't sure if it might still be used at some point (maybe for debugging?), so that can be reverted if necessary.
I realize this might take a while to look through, but I hope you get around to it at some point.