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

Test fix for the static event emitter #161

Closed
wants to merge 2 commits into from
Closed

Conversation

agsh
Copy link
Owner

@agsh agsh commented May 15, 2020

No description provided.

@agsh agsh requested a review from RogerHardiman May 15, 2020 14:49
@agsh agsh self-assigned this May 15, 2020
@bl0ggy
Copy link
Contributor

bl0ggy commented May 15, 2020

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
Copy link
Owner Author

agsh commented May 15, 2020

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
Copy link

dmitrif commented Oct 2, 2020

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

@agsh agsh closed this Dec 25, 2020
@agsh agsh deleted the fix-static-EventEmitter branch October 12, 2022 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants