Hot reload stop observing issue

[Romick2005]

Skip stopObserving because in debug mode startObserving is not called on reload, but stopObserving is called before reload.

My application fetch voip token every time on start, but when working on UI with hot reload I have an annoying warning that voip token was not fetched from native code. That is because somehow on hot reload stopObserving is called, but then no startObserving is called. This might be some react-native issue.

So I think this code does not have any harm, but fix unexpected react-native observer behaviour.

If anyone has any objection please let me know.

[zxcpoiu]

My previous comments are here: #87 (comment)

[jonastelzio]

@zxcpoiu @Romick2005

Yeah the issue with reloading is also suffered by CallKeep (and I'm sure a lot of other RN modules around the globe). I've pointed out the root cause, and a solution here: react-native-webrtc/react-native-callkeep#406

To quickly summarize, it's because of this change to react native: LINK

It used to be that react native relied on deallocate to run logic to execute stopObserving. Now that logic has moved to invalidate. HOWEVER, because both this module and CallKeep implements the RTCEventEmitter interface into a class that's instantiated as a singleton, this module can never be deallocated (and stopObserving was NEVER called btw), and as such the internal variable _listenerCount is never reset back to zero, and they explicit make this check in their code:

  _listenerCount++;
  if (_listenerCount == 1) {
    [self startObserving];
  }

I've suggested a proper solution to this in the issue over on the call keep repo.

[Romick2005]

Hi @jonastelzio thank you for clarification. Sounds good to me to have RNCallKeepRTCEventEmitter instead of RTCEventEmitter with a static function registerRTCEventEmitter. That is a good way to go.

But I have also some idea and thinking of why invalidate function of RTCEventEmitter does not explicitly set _listenersCount to 0? Is there any reasons not to do so?

Or why don't just expose removeListeners to the native part and then those function could be used to reset _listenersCount.

Yes I understand that trying to apply this change in react-native would require their approve and take longer than expected. But if it the right thing to do then why not? I am convinced that we should fix the root of the problem, because creating a wrapper for callKeep will not fix those issue for other libraries.

I can also be wrong in my way of thinking as I might not see the whole picture that was set up behind.

[jonastelzio]

@Romick2005

But I have also some idea and thinking of why invalidate function of RTCEventEmitter does not explicitly set _listenersCount to 0? Is there any reasons not to do so?

Looking at the code, it's clear that facebook expects to be able to manage the life cycle of the modules. That's fine, during an invalidation (dev mode reload), the modules call invalidate and then deallocate. I assume that deallocate is in fact called by the underlying runtime rather than explicitly.

The issue is that by implementing the RN Module interfaces into an object, that is then instantiated as a singleton, completely breaks with facebooks assumptions that they can rely on deallocation - and it's sort of creating an unexpected situation for react native.

When RN does a reload, it's not like it performs a proper teardown of the react components, so stuff like removeEventListener will never be called. It simply cycles the entire runtime and executes the entry point again, so exposing removeEventListener wouldn't help us. We simply need to allow react native to manage these instances as it was designed to.

It's not a difficult change. Just need to have the react native modules register themselves with our singleton when they initialize, and unregister on deallocate/invalidate.

The reason this worked before RN made that change, is that deallocate was never called, and stopObserving was never called. If you apply some breakpoints in a pre 0.64, you'll see that the _listenersCount variable just increases every upwards on every reload.

[Romick2005]

Ok. Let me roll out another thought.

In this particular module we use _hasListeners only to not miss some events (collecting them in _delayedEvents), that would not be captured by JS, but already happened in native part. I think this module does not need stopObserving as it is singleton. So if at least one listener were added on JS side that mean that we can fire any events freely even if there will be some invalidation that will call stopObserving, we should not care of that as our singleton is already created and ready to fire any events to the JS side. I believe that while singleton are available JS part that was at least one time linked (some listeners were added) should be also available. Would that work @jonastelzio or there are some other pitfalls?

@zxcpoiu Can you please also take a look at this and review -> merge?

[jonastelzio]

@Romick2005 I see what you're getting at, and yes it would work, for our implementations, with the current version of react native.

But it's not a solution. If the RN maintainers decide to change this behavior, it'll break our module.

[Romick2005]

I would argue with you on this again "if RN maintainers decide to change smth" that will definitely break smth :). So that is fragile argument I would say. I see you point also, and agree that having our custom wrapper for RNCallKeepRTCEventEmitter would be very good in a way to have more control over code, but it is not needed if we decided to go with singleton and also require more code :) (more complex code -> more vulnerability) :).

But anyway this is my retro perspective point of view (my way of thinking). For now I would like to have this limited, but workable solution, unless anyone implements RNCallKeepRTCEventEmitter.

Thank you @jonastelzio for your opinion. It was very valuable.

[jonastelzio]

@Romick2005 I think it's pretty clear from looking at the react native code, that you simply aren't meant to implement the module interfaces into singletons at all.

While they don't mention this in the documentation, it's pretty obvious that something like RN needs to be able to instantiate objects in a predictable fashion.

[Romick2005]

Ok. So you suggest to remove singleton pattern from CallKeep module? What is the right direction to move here?