Signal events are delivered twice

[ben-i2x]

Bug Report

Current behavior

Signal events are being dispatched twice for the same type.

Steps to reproduce

In order to receive signal events I needed to register an event handler with the type of signal event. For example, consider signal events with type set to 'chat':

     this.session.on({
      streamCreated: (event) => {
        this.session.subscribe(event.stream, 'your-video');
      },

      streamDestroyed: (event) => {
      },

      'chat': function(evt) {
        var chat = document.getElementById('chat');
        var p = document.createElement("p");
        p.innerHTML = evt.data;
        chat.appendChild(p);
      }
    });

This differs from the signal sample project where an event is listened to with the notation: 'signal:chat'.

When the signal is received this code is executed:

TBSession.prototype.signalReceived = function(event) {
  var streamEvent;
  streamEvent = new TBEvent("signal");
  streamEvent.type = event.type; // <<<<<
  streamEvent.data = event.data;
  streamEvent.from = this.connections[event.connectionId];
  this.dispatchEvent(streamEvent);
  streamEvent = new TBEvent("signal:" + event.type);
  streamEvent.type = event.type; // <<<<<
  streamEvent.data = event.data;
  streamEvent.from = this.connections[event.connectionId];
  return this.dispatchEvent(streamEvent);
};

Note that it constructs two TBEvent objects, one with the type 'signal' and one with 'signal:' +event.type.

However, both events have their type explicitly set to event.type, overwriting this distinction.

The result is that two events for the same type are triggered, rather than two different events.

Removing the explicit setting of streamEvent.type (the lines noted above) fixes this issue.

Example Project

None provided. If this is necessary, I'll create one.

What is the current bug behavior?

See above.

What is the expected correct behavior?

See above.

[msach22]

@ben-i2x I'm not sure if I understand correctly. The API only allows you to listen to the signal events using the signal or signal:type event, it does not support type without the signal preface.

As for the signal being dispatched twice, it's how the OpenTok JS SDK works as well. For example, if we dispatch a specific event with type set to chat, it will dispatch the event with the chat and dispatch a standard signal event because it's still a signal. Essentially, by using type, you are just being more specific on the type of event that you want to listen to.

[ben-i2x]

The API only allows you to listen to the signal events using the signal or signal:type event, it does not support type without the signal preface.

I understand this is how it's supposed to work. I'm saying that if you look at the current code there's a bug that keeps it from working this way.

 TBSession.prototype.signalReceived = function(event) {
    var streamEvent;
    streamEvent = new TBEvent("signal");
    streamEvent.type = event.type; // [1]
    streamEvent.data = event.data; 
    streamEvent.from = this.connections[event.connectionId];
    this.dispatchEvent(streamEvent);
    streamEvent = new TBEvent("signal:" + event.type);
    streamEvent.type = event.type; // [2]
    streamEvent.data = event.data;
    streamEvent.from = this.connections[event.connectionId];
    return this.dispatchEvent(streamEvent);
  };

Note lines [1] and [2] above.

They are overwriting the value of streamEvent.type, and making it so that the event dispatched are duplicates.

I apologize for the confusing title. I understand that two events are supposed to be dispatched (one for 'signal' and one for 'signal:chat') - my point is that the code is dispatching two of the same type event.

Maybe I'm using the wrong source code or something? I'm trying reference the latest code.

[msach22]

@ben-i2x My apologies for not understanding. What should be happening is that the event should be only fired once, but you should be able to listen to it either as signal or signal:type or both if you wanted to. It should not be firing twice.

Is this what you're suggesting as well?

[ben-i2x]

What should be happening is that the event should be only fired once, but you should be able to listen to it either as signal or signal:type or both if you wanted to.

This is a sound strategy.

My point is that there's a bug in the code that's making it so that it doesn't work this way. The current code (as far as I can tell) issues two events both with type set to 'type' (vs: 'signal' and 'signal:type').

I described the bug in terms of the fact that the same type of event was being issued twice. But another way to look at this bug is that the correct type is not being set on the dispatched signal events.

Either way, this should be trivial to fix.

[msach22]

@ben-i2x I reread the issue and I'm not sure if I answered your questions properly. Both of the events should have the exact same type and data. It's just that they have two different names: signal or signal:type because at the core asignal:type event is still a signal event. This behavior is not a bug, it's intentional.

I just ran a quick test and sadly neither the signal nor the signal:type events were firing :/ I'm currently using plugin version 3.2.1. Although this is a separate issue, could you please let me know if the signal or signal:type events are firing for you?

[ben-i2x]

I reread the issue and I'm not sure if I answered your questions properly. Both of the events should have the exact same type and data. It's just that they have two different names: signal or signal:type because at the core asignal:type event is still a signal event. This behavior is not a bug, it's intentional.

I understand this, and this bug report I filed has nothing to do with this. However, it has everything to do with what you're seeing below.

I just ran a quick test and sadly neither the signal nor the signal:type events were firing :/

Excellent! This is what's happening to me.

Can you try two things:

1. Register an event handler for just 'type'. So instead of:

   session.on({'signal' : ..., 'signal:foo' : ...{)

try:

session.on({'foo' : ... })

I know that this isn't how the code is supposed to work. Can you try it anyway?

2. Can you manually edit opentok.js and remove the lines noted here:

   TBSession.prototype.signalReceived = function(event) { var streamEvent; streamEvent = new TBEvent("signal"); streamEvent.type = event.type; // DELETE ME streamEvent.data = event.data; streamEvent.from = this.connections[event.connectionId]; this.dispatchEvent(streamEvent); streamEvent = new TBEvent("signal:" + event.type); streamEvent.type = event.type; // DELETE ME streamEvent.data = event.data; streamEvent.from = this.connections[event.connectionId]; return this.dispatchEvent(streamEvent); };

Does that fix the problem?

I'm currently using plugin version 3.2.1. Although this is a separate issue, could you please let me know if the signal or signal:type events are firing for you?

They are not firing for me. And this isn't a separate issue - this is exactly the issue I'm having.

[msach22]

@ben-i2x Great catch and explanation :) As you said, when we set streamEvent.type, it overrides the event name. If you're up for it, please send a PR, if not, I'm happy to fix the issue myself! Please keep in mind that the issue will be fixed in the 3.3.0 release which should be going out mid-next week.

[ben-i2x]

@msach22 - awesome, I'm glad the fix worked!

I'm signing off for the weekend, so I don't know when I'll get a chance to create a PR. You should probably just fix it.

Thanks for working with me on this and for the hard work on in general on cordova-plugin-opentok!

[ben-i2x]

@msach22 - I just realized I have no idea what you're preferred protocol for opening/closing these tickets and I just closed it.

So I'm re-opening this ticket and I'll leave it to you close. Sorry for any confusion.

[msach22]

@ben-i2x Thanks for keeping the issue open! We like to keep the issue open until the fix has been released. Since this is a P1 bug, I will be releasing a patch and releasing this with 3.2.2 by EOD

[ben-i2x]

@msach22 - thanks for the explanation and the fix.

[msach22]

Thanks for all your help @ben-i2x! Updated and released on NPM with 3.2.2!