agsh / onvif

ONVIF node.js implementation
http://agsh.github.io/onvif/
MIT License
699 stars 240 forks source link

Test fix for the static event emitter #161

Closed agsh closed 3 years ago

bl0ggy commented 4 years ago

This is much better than my dirty PR, I didn't know it was that simple...

I don't have access to cameras for now, but this simple test shows that it works:

const events = require('events');
const util = require('util');

let Cam = function(name) {
    this.name = name;
    // events.EventEmitter.call(this);
    // this.on('newListener', function (name) {
    //     console.log(this.name,'newListener');
    //     if (name === 'event' && this.listeners(name).length === 0) {
    //         console.log(this.name,'first event listener');
    //     }
    // });
}

util.inherits(Cam, events.EventEmitter);

Cam.prototype.on('newListener', function (name) {
    console.log(this.name, 'newListener');
    if (name === 'event' && this.listeners(name).length === 0) {
        console.log(this.name, 'first event listener');
    }
});

let a = new Cam('a');
let b = new Cam('b');

a.on('event',()=>{});
b.on('event',()=>{});

This does not print "b first event listener" (meaning that it does not work), but after commenting Cam.prototype.on and uncommenting what is inside Cam constructor, we see both "a" and "b".

However I would like you to check about a few things (and update this PR) as they are related to events:

  1. For lines 204 and 251 there are try/catch made for the subscriptionId so I think we should use the variables created for that purpose.
  2. Line 262, as we create a PullPointSubscription when adding a listener, we should remove all listeners or we won't be able to subscribe again. This also fixes the infinite loop explained next.
  3. I remember having trouble with an infinite loop _eventRequest/_eventPull/pullMessages while unsubscribing when waiting for the response of pullMessage (when we get a response from pullMessage we send another one, or if the subscription timeout is over, create a new PullPointSubscription, which is not what we want because we just unsibscribed). It seems that I added a condition here but I can't find out how this can fix the infinite loop. So if you don't accept to remove all listeners when unsubscribing, we will have to find a way to stop that loop.
agsh commented 4 years ago

Ok, I'll merge our branches with all common changes in the next week. And then publish it to the npm. Thanks for the quick response!

dmitrif commented 4 years ago

Hi all! Just wanted to follow up on the status of this. Thank you.