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

Stop relaying on errors #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pavelvasev
Copy link

No description provided.

@colinbdclark
Copy link
Owner

Hi @pavelvasev, thank you very much for the pull request. Can you elaborate a bit more on the issue you're facing, and how to reproduce it? I'd like to better understand what the current problem is and why it occurs. Thanks!

@pavelvasev
Copy link
Author

Hi @colinbdclark !

Thank you for your fast response. Please consider an example:

  1. Start nodejs udp server relaying to websockets, as in your example.
  2. Start some websockets client, also from same example.
  3. Begin sending any osc-udp message to the server (1) => it will relay messages to browser client (2), all is great.
  4. Reload the browser page of the client (2) => and on next message (3), you will get server (1) crash, because of network exception like "failed to send to client, the websocket is dead".

@colinbdclark
Copy link
Owner

Okay, thanks. I think I understand this issue more clearly now.

So, with Node.js EventEmitters (including dgram.Sockets), if no "error" listeners are registered and an error occurs, the defined behaviour is to throw. This is why you're seeing the behaviour you are.

Shouldn't your code be listening for errors in the ports you're relaying and responding appropriately? (i.e. by trying to reestablish the connection or by tearing down the relay and showing the user an error message?) Or, put differently, shouldn't the osc.js examples also be doing this? My bad.

The factoring of osc.Relay isn't quite helpful in many of these cases, and I expect it will be refactored for the 3.0.0 release in order to make its behaviour more clear. In the meantime, however, we could implement a contract that mirrors the behaviour of "close" events on either side of the relay, which involves closes both ports and the relay itself in case either side closes.

So, in this case, I think we should be registering "error" event listeners on each port in the relay upon instantiation, and then stopping the relay and firing an "error" event from the osc.Relay instance so that clients can respond appropriately.

Does this make sense? And if so, do you want to go ahead and make the appropriate changes to your pull request?

Thanks again!

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.

2 participants