i8beef / node-red-contrib-castv2

MIT License
22 stars 14 forks source link

EventEmitter warning #23

Closed i8beef closed 4 years ago

i8beef commented 4 years ago

Node issues an EventEmitter warning as there are over 10 subscriptions to some event down in the castv2 or castv2-client library. This may be ok, and there is a way to turn this number up if that is to be expected.

From initial investigation, none of the subscriptions in the actual castv2-sender or castv2-connection nodes are oversubscribed, even after multiple reconnects, cast events, etc.

A review of the code in castv2 and castv2-client might be a good idea to ensure they are properly disposing of their event registrations. If they are found to be ok, it might be a good idea to see if the EventEmitter in question can have its warning threshold upped from calling code (i.e., without necessitating getting a hold of the devs that seem to be MIA on those projects) in order to not worry users who see this warning.

i8beef commented 4 years ago

Tracked down to several areas.

  1. castv2-client.DefaultMediaReceiver opens a media channel, but never closes it. There is a message bus buried down under the Channel class that has attached events, and shares a lifetime with the main Client: ergo if you reuse Client, but destroy Receivers (and their controller channels) those start orphaning connected events to that bus. DefaultMediaReceiver should have its own "close" method that cleans it up, which should always be called like the underlying Application's connection channel. Needs fixed upstream, or need to reimplement with our own.
  2. The same "close" should be added to our own Receivers as well, and the node should be made to call this any time it is abandoning a receiver instance.
  3. The CastV2Sender.join method is ALWAYS joining, regardless of if it is already joined or not. Add a check to only execute when receiver is null or not the same app as is being requested to join.
  4. Receiver lifetime management has a race condition due to the "auto join active session" functionality when launching a new app. A launch will return a joined receiver that sendCommandAsync will initReceiver on, caching that instance, but the platform status update from the chromecast will arrive in that time and trigger a "tell all nodes to join this session". This is fine for node instances that DIDN'T trigger the launch, but the one that DID needs a lock around the launch event, and any triggered join on that node needs no-op while in the middle of a launch.
  5. castv2-client.MediaController calls self.stop() on self emitted close, which attempts to send a STOP session command through the media channel. That close is inherited from Controller, which when called properly now here to dispose of attached Client bus events, means the channel has already gone away, so its gonna error. This needs fixed upstream, or need to reimplement with our own.