Replace events for pr

[rlsutton1]

This is a very large PR...

I've replaced the logger with a new logger which outputs the time, log level, source file and line number of the log statement that created the logger line.

[2019-10-11 12:18:26.910] Level.debug WebSocketInterface.dart:161 ::: send()

I've replaced events2 (the message bus) with a completely new and fully typed (as much as is currently possible) event bus called EventManager, this has been a very invasive change and as such I have spent a lot of time making sure things still work and have undergone a full internal code review of every line of changed code.

It is now possible to statically determine which parameters are available when receiving an event and which parameters must be passed when emitting an event.

handlers.on(EventOnSuccessResponse(), (EventOnSuccessResponse event) {
logger.warn("$event.response");
});

[ghenry]

Excellent work. Is this a major change from his JsSIP do thing or just related to the Flutter way?

[rlsutton1]

Untyped languages like java script make it easy introduce bugs.

Dart/flutter supports types so it makes sense to make these changes for the long term benefits.

With the addition of type information many errors are found by the compiler as soon as the code is changed.

With this code change, a number of errors and unused code became apparent.

[ghenry]

Understood. I don't think JsSIP uses Typescript.

[cloudwebrtc]

For the log datetime:level:file:line:func prints format is very good idea.

For the modification of the internal EventHandlers in the stack, I think it is necessary because I used the Map<dynamic,dyanmic> method to build the function list in order to make the stack work earlier. Maybe we can create an EventHandler class with a fixed list of functions.

I think this PR is too radical for the change of event manager. I want to keep it similar to the JsSIP API in the early version(JsSIP code is stable), focusing on the main function implementation (v 1.0 in the project), that is, keeping the code simple and not increasing in functionality. In this case, try not to increase the number of source files too much.

We can consider doing a gradual iteration while the first version is stable.

So I suggest:
1, You can keep the changes to the logger.
2, Modify the EventHandlers inside the stack.
3, After the completion of the 1.0 stable version, discuss the refactoring of the event manager.
So I suggest that we can open some issues first and then discuss the need for changes.

[rlsutton1]

What this PR is about

I was motivated to make this PR as it is very difficult to reason about where events go and actually what information is available when receiving an event. With this PR, incorrect types at either the "emit" or "on" method result in a complier error - much better a compile error than a runtime error.

What this PR does
A number of bugs were discovered during this conversion, as with the now included types, the compiler was able to highlight a number of problems. In many places the compiler generated errors where events with variables of the wrong type were being sent and variables that were expected by the "on" methods were not being sent. These things would definitely lead to runtime exceptions which were impossible to predict. But with this PR the compiler tells you about them without even needing to run the code.

It also shows how it is very difficult to manage the code base effectively without type information. The compile time checks have made it easy to determine that much of the data being passed around isn't even being used (have a look at all the variables I commented out in event_manager/events.dart, these are all "emitted" but never used anywhere) , which simply adds unnecessary complexity to the code.

Points for discussion...

keeping the code simple and not increasing in functionality

While the new "emit" and "on" code is a little more verbose and may be perceived as complexity, it greatly improves the static analysis for the project. It does not add new functionality.

2, Modify the EventHandlers inside the stack.

Can you clarify what you mean by "inside the stack" do you mean all the code not in the example project or do you mean inside of Events2?

Maybe we can create an EventHandler class with a fixed list of functions.

I'm uncertain how this would add any value? I can't see any way that this approach would connect the method signatures and type information of the "emit" calls to the "on" calls and improve the static analysis (this is the entire point of this PR).

After the completion of the 1.0 stable version, discuss the refactoring of the event manager

This seems to be the wrong approach to me, currently there are no projects dependant on this project and breaking it now won't effect anyone. If we wait until 1.0 it will cause a lot of problems for everyone using it. Like you, we are looking for the best way to achieve a stable stack. We viewed this changes as critical to achieving that. The existing code based made it extremely difficult to reason about the code base in order to fix the bugs we were finding. The stack is also likely to get the most testing pre 1.0 than it will get after. We joined this project as we are heavily invested in getting a reliable sip stack. Whilst I do agree that the changes a fairly invasive the net benefit seems to significantly outweigh the risk.

I think this PR is too radical for the change of event manager. I want to keep it similar to the JsSIP API in the early version(JsSIP code is stable)

With this PR it still remains very similar to jsSip, specifically I've replaced the logger (which you've already agreed to take), I've added a new EventManager class which is just a replacement for Events2 and other than that every other change is basically just replacing every "on" and "emit" line with it's EventManager based replacement - which is quite similar. There are admittedly a few bug fixes I've included but otherwise the code remains unchanged.
When undertaking an internal code review other team members were easily able to understand the mappings from the old event map to the new one.

I have also had a look at the state of the JSSip api. It currently sits at version 3.x. Changes to the stack appear to be small and infrequent which suggests that the stack is extremely stable.
As such we don't expect there too much difficulty in back porting changes from JSSip to this project.
i.e. changes from JSSip are expected to be small and infrequent.
The changes that we have made are largely constrained to creating a new event manager.
Where these changes impact other files the change is simply a mapping from the old event manager api to the new one.
As such we believe that merging JSSip changes will be simple.

[cloudwebrtc]

In fact, what I want is to keep the EventEmitter classic style, but we can also limit the parameter types in the event function, So is it possible to directly modify the dart-events repo to add type checking, and still retain the original calling style?

Like this:

class EventEmitter {

class Listener {
    Function func;
    List<Type> typeOfArguments;
    Listener(this.func);
    checkTypeForArguments(arg0, ...) {
   }
};

Map<String, List<Listener>> _events;

void on(String event, Function(dynamic arg0 ...) func) {
     Listener listener = Listener(func);
     listener.typeOfArguments.add(arg0.runtimeType);
     listener.typeOfArguments.add(arg1.runtimeType);
     ....
     _events[event].add(listener);
}

void emit(String event, dynamic arg0 ...)  {
    _events[event].forEach((Listener listener) {
      try {
        // check all types of arguments.
         listener.checkTypeForArguments(arg0 ...);
         // call function.
         listener.func(arg0, ...);
      } catch (e){
         Throw e;
      }
   });
}

}

In the long run .on() .emit() has only one function argument and is not flexible enough.
So if you can directly modify the dart-events repository, I think it will be the perfect solution.

[cloudwebrtc]

Can you clarify what you mean by "inside the stack" do you mean all the code not in the example project or do you mean inside of Events2?

I mean this part of the code really needs to be modified.
https://github.com/cloudwebrtc/dart-sip-ua/pull/23/files#diff-c7fdeeb1b54dea7b19d9c85dca9b5561L11-L18

This 'event_tag': Fuction() map is very ugly.
Similar to this:

var EventHandlers = {	
  'onRequestTimeout': () => {},	
  'onTransportError': () => {},	
  'onSuccessResponse': (response) => {},	
  'onErrorResponse': (response) => {},	
  'onAuthenticated': (request) => {},	
  'onDialogError': () => {}	
};

and other code like this.

send() {
    var request_sender = new RequestSender(this._ua, this._request, {
      'onRequestTimeout': () => {this._eventHandlers['onRequestTimeout']()},	    
      'onTransportError': () => {this._eventHandlers['onTransportError']()},	        
      'onAuthenticated': (request) => this._eventHandlers.emit(EventOnRequestTimeout());
      'onReceiveResponse': (response) => {this._receiveResponse(response)}	
    });

[rlsutton1]

In fact, what I want is to keep the EventEmitter classic style, but we can also limit the parameter types in the event function, So is it possible to directly modify the dart-events repo to add type checking, and still retain the original calling style?

Like this:

class EventEmitter {

class Listener {
    Function func;
    List<Type> typeOfArguments;
    Listener(this.func);
    checkTypeForArguments(arg0, ...) {
   }
};

Map<String, List<Listener>> _events;

void on(String event, Function(dynamic arg0 ...) func) {
     Listener listener = Listener(func);
     listener.typeOfArguments.add(arg0.runtimeType);
     listener.typeOfArguments.add(arg1.runtimeType);
     ....
     _events[event].add(listener);
}

void emit(String event, dynamic arg0 ...)  {
    _events[event].forEach((Listener listener) {
      try {
        // check all types of arguments.
         listener.checkTypeForArguments(arg0 ...);
         // call function.
         listener.func(arg0, ...);
      } catch (e){
         Throw e;
      }
   });
}

}

In the long run .on() .emit() has only one function argument and is not flexible enough.
So if you can directly modify the dart-events repository, I think it will be the perfect solution.

The above proposed change ensures that the listener receives what it expects, but this check will only happen at run time (resulting in an Exception), which means we have no way of reliably finding all of these problems before releasing the code. It also does not ensure that the "emit" code sends what the listener expects and it does not enforce that all listeners for a particular type of event expect the same arguments.

Currently the system passes types that are wrong or absent, but most of the time they don't cause problems. The proposed change will mean that they always cause Exceptions.

It is not possible to fix these issues while confining code changes to the EventEmitter class.

In the long run .on() .emit() has only one function argument and is not flexible enough.

The EventManager in this PR takes one argument derived from EventType, that argument is a class which can be used to pass any number of arguments. I've defined many of these, one for each eventType, for example EventCallState which currently allows passing of 6 fully typed values, and can easily be modified to pass an un-limited number of fully typed values including Functions if required. Given the flexibility of this model it should be able to handle any future requirements.

class EventCallState extends EventType {
  String state;
  // dynamic response;
  String originator;
  MediaStream stream;
  bool audio;
  bool video;
  EventCallState(
      {this.state,
      dynamic response,
      this.originator,
      this.stream,
      this.audio,
      this.video});
}

example emit:

    this.emit(EventCallState(
        state: state,
        response: response,
        stream: stream,
        originator: originator,
        audio: audio,
        video: video));

example on:

this.on(EventCallState(),EventCallState data) {
    if (data.state == 'hold' || data.state == 'unhold') {
      _hold = data.state == 'hold';
      _holdOriginator = data.originator;
      this.setState(() {});
      return;
    }
}

The code in this PR implements Static typing and immediately reveals problematic paths of this nature without ever needing to run the code! Thus removing all of these types of problems.

[rlsutton1]

Can you clarify what you mean by "inside the stack" do you mean all the code not in the example project or do you mean inside of Events2?

I mean this part of the code really needs to be modified.
https://github.com/cloudwebrtc/dart-sip-ua/pull/23/files#diff-c7fdeeb1b54dea7b19d9c85dca9b5561L11-L18

This 'event_tag': Fuction() map is very ugly.
Similar to this:

var EventHandlers = {	
  'onRequestTimeout': () => {},	
  'onTransportError': () => {},	
  'onSuccessResponse': (response) => {},	
  'onErrorResponse': (response) => {},	
  'onAuthenticated': (request) => {},	
  'onDialogError': () => {}	
};

and other code like this.

send() {
    var request_sender = new RequestSender(this._ua, this._request, {
      'onRequestTimeout': () => {this._eventHandlers['onRequestTimeout']()},	    
      'onTransportError': () => {this._eventHandlers['onTransportError']()},	        
      'onAuthenticated': (request) => this._eventHandlers.emit(EventOnRequestTimeout());
      'onReceiveResponse': (response) => {this._receiveResponse(response)}	
    });

The hyper-link just points to the entire diff, so I'm still not sure what this is about

[cloudwebrtc]

The above proposed change ensures that the listener receives what it expects, but this check will only happen at run time (resulting in an Exception), which means we have no way of reliably finding all of these problems before releasing the code. It also does not ensure that the "emit" code sends what the listener expects and it does not enforce that all listeners for a particular type of event expect the same arguments.

I don't think that using an instance of a type as an event tag can limit the occurrence of errors. It just uses eventInstance.runtimeType as the tag of the event. When we develop it, we should know that an event tag corresponds to a specific function, so Limiting the type of listener does not prevent exceptions. For example, when we emit an event, if we use the wrong type, it will also trigger an error.

Instead I think EventManager has increased the amount of code.
For example: EventOnTransportError
Before modification

/// trigger event:
_eventHandlers.emit('onTransportError');
/// Receive events:
_eventHandlers.on('onTransportError', () {
         // got events.
   });

after modification:

/// trigger event:
_eventHandlers.emit(EventOnTransportError());
/// Receive events:
_eventHandlers.on(EventOnTransportError(), (EventOnTransportError event) {
         // got events.
   });

In this case, an event with no function arguments, we still have to create a class for it.

I am in favor of combining some event maps into classes, but I don't think it makes sense to use types as event trigger parameters.

In addition, we'd better submit several different PRs separately, such as log and event as separate PRs.
Committed commits can cause conflicts and make existing code unstable. Because when I tested your branch, the call became abnormal (initiating a video call, becoming an audio call), and maybe there were other problems.

[cloudwebrtc]

Can you clarify what you mean by "inside the stack" do you mean all the code not in the example project or do you mean inside of Events2?

I mean this part of the code really needs to be modified.
https://github.com/cloudwebrtc/dart-sip-ua/pull/23/files#diff-c7fdeeb1b54dea7b19d9c85dca9b5561L11-L18
This 'event_tag': Fuction() map is very ugly.
Similar to this:

var EventHandlers = {	
  'onRequestTimeout': () => {},	
  'onTransportError': () => {},	
  'onSuccessResponse': (response) => {},	
  'onErrorResponse': (response) => {},	
  'onAuthenticated': (request) => {},	
  'onDialogError': () => {}	
};

and other code like this.

send() {
    var request_sender = new RequestSender(this._ua, this._request, {
      'onRequestTimeout': () => {this._eventHandlers['onRequestTimeout']()},	    
      'onTransportError': () => {this._eventHandlers['onTransportError']()},	        
      'onAuthenticated': (request) => this._eventHandlers.emit(EventOnRequestTimeout());
      'onReceiveResponse': (response) => {this._receiveResponse(response)}	
    });

The hyper-link just points to the entire diff, so I'm still not sure what this is about

I mean EventHandlers using EventEmitter or EventManager replacement is a good idea, I am in favor of modifying these ugly 'event_tag': Function structs.

[rlsutton1]

I don't think that using an instance of a type as an event tag can limit the occurrence of errors.

It makes it harder to accidentally use an incorrect event label like 'onnecting', instead of "onconnecting" as is present in sip_ua_helper.dart line 80

Below you can see I have deliberately changed cause to caused and the compiler is flagging an error, if this were in a map there would be no such error until the code blew up at runtime.

Here you can see I've changed EventEnded to EventEnds and the compiler is flagging an error, if we use strings instead of classes there will be no error and it will silently fail

Here you can see I'm trying to pass an int (state) where a String is expected and the compiler is flagging an error, if we use maps there will be no error and we will get either unexpected behaviour or an exception at runtime.

The lack of type information around objects in maps when reading code is very difficult to deal with. Running the code and managing to get it to go through the correct path for testing is difficult and time wasting.

Additional type information speeds debugging up significantly.

[cloudwebrtc]

Thanks, I think what you said makes sense, I will try to merge it after passing the test.

[rlsutton1]

OK, I've just pushed a 1 line commit to this PR branch to fix a bug I found

[rlsutton1]

I've created a pull request on dart-sdp-transform which is required here

[rlsutton1]

I've been doing quite a bit of testing, and have added a number of fixes for this PR

[ghenry]

How is our automated testing / ci? We could mock a sip / webrtc server for integration testing.

…

On Fri, 18 Oct 2019, 07:19 Robert Sutton, ***@***.***> wrote: I've been doing quite a bit of testing, and have added a number of fixes for this PR — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#23>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABG66BVECSIXIW4MPK2DSDQPFIOTANCNFSM4I7T2PXQ> .

[cloudwebrtc]

LGTM