-
Notifications
You must be signed in to change notification settings - Fork 240
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
Fix EventEmitter global to all Cam objects. #137
Conversation
Avoid subcription loop when we unsubscribe before pullMessage returns a response.
Fixed test failures.
Just a thought (and I've not had much time to look at this). |
That would be better of course, but that would break the code of people using NodeJS below v6. Maybe for version 1.0.0 of this repository. |
Have invited Andrew and Chris to review |
@bl0ggy I'm seeing an issue with the current codebase where when multiple cam objects are created and I register to listen to each using the |
Answering my own question above: from some initial testing it does look like this fixes the issue I described |
@brilthor Yes this PR fixes the issue you explain. The problem is that the |
Many thanks for the change and the PR. |
@bl0ggy @RogerHardiman Hi! And very big sorry for the late reply and I'll maybe revert this merge. This is definitely my huge mistake to wrote this in
And you are absolutely right in putting subscription to the But are you sure, that we need to add |
Hi @agsh So if you need to revert and change the code, then go ahead. |
@brilthor @bl0ggy @RogerHardiman |
Using
util.inherits
makesEventEmitter
global to allCam
objects.It is recommended to
extend
instead of usingutil.inherit
, which fixes the problem, but it is not possible as we don't useclass
es.This pull request fixes that by instantiating an
EventEmitter
in theCam
constructor and creatingon
andemit
functions.I also added a condition on listeners in
_eventRequest
to avoid_eventRequest
/_eventPull
/pullMessages
loop when we unsubscribe while waiting for apullMessages
response (which is almost always the case). The listeners are also removed inunsubscribe
because we could never subscribe anymore as we create a PullPointSubscription only if there is no listeners.The code below shows that the
EventEmitter
is global. Also include aconsole.log
of the listeners length here, right afterCam.prototype.on('newListener', function(name) {
to see how it increases as you add more cameras.To test the loop, keep only one camera with
on('event')
, then add asetTimeout()
that will unsubscribe the camera after a while, and addconsole.log
s at the begining of functions_eventRequest
,_eventPull
andpullMessages
.