Closed bl0ggy closed 4 years ago
Just a thought (and I've not had much time to look at this). But should we convert the Cam object into a proper JS Class?
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 cam.on('event', ...
after a time only one remains in the polling loop. Is this what is fixed by this MR or is that an unrelated issue?
Hi all and especially to @bl0ggy and @brilthor @bl0ggy has done lots of Pull Requests which all look good and make lots of improvements. I've been so busy recently since the lockdown in the UK that I've not had time to go through and merge them all yet. Sorry about that.
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 EventEmitter
is common to all Cam
objects. When you try to listen to event
with one camera, we count how many listeners are listening to that event, if the count is 0 we create a PullPointSubscription. So when you try to listen to event
with another camera, the count will not be 0 anymore as the EventEmitter
is the same for all cameras, so no PullPointSubscription is created anymore.
Many thanks for the change and the PR. Sorry it took a while to merge.
@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 events.js
:
Cam.prototype.on('newListener', () => { ... });
And you are absolutely right in putting subscription to the newListener
event into the constructor. And this is the bug point.
But are you sure, that we need to add eventEmitter
property? IMHO we can refuse it and everything should work ok. Maybe also add EventEmitter.call(this);
at the top of the constructor.
Hi @agsh Thanks for the feedback. I had a quick look at the PR, ran a test and accepted the merge after there was a 2nd user hitting the bug.
So if you need to revert and change the code, then go ahead.
@brilthor @bl0ggy @RogerHardiman I think, I found a better solution. Can you please check and review this PR https://github.com/agsh/onvif/pull/161? If it works and fixes the problem with two and more cams it'll be great!
Using
util.inherits
makesEventEmitter
global to allCam
objects. It is recommended toextend
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 anEventEmitter
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
.