Queue async actions

[kenny-house]

This PR adds a queue to ensure that actions with async steps (like SRD -> SLD) run synchronously, helping avoid SLD failures.

The following functions were not updated as part of this PR:

1. addStream2
2. removeStream2
3. onIceCandidate
4. onIceEndOfCandidates
5. onIceStateChange
6. onAddStream
7. onRemoveStream
8. onAddChannel

addStream2 and removeStream2 were not updated because they simply wrap addStream and removeStream (which were updated). The remaining functions were not updated because they are primarily wrapping emitted events from the peer connection.

I'm using jingle-media session with sessions initiated by our XMPP server to connect clients to our SFU. With that in mind, I have not yet tested these changes in a p2p context. If anyone can give this PR a test in p2p contexts, that would be helpful. I plan on setting up an application to test that as well, but it will likely be a few days before I can get that up and running.

[xdumaine]

@kenny-house Thank you for your contribution! This is very helpful. I'm reviewing this today. I've been meaning to get around to something like this for a while and haven't had a chance to work on it.

[kenny-house]

Sure thing! I want to mention that I'm not convinced the style / format I've submitted is the absolute best way to go about queueing everything. It does strike me as a simple way to make the change, though. It's particularly odd having a few instances of function(cb, done). It does preserve the current logic that's been battle-tested, though.

Also, I'm not sure if handling this a layer up (Jingle) might be preferable, I'm not familiar with that side of things or if other session types might benefit from queueing.

I'm happy to change and fix as needed or as preferred!

I have this code running in our staging environment right now so we're getting some more testing with our devs. Not a single SLD failure so far!

[xdumaine]

I think the overall strategy looks good. There are some simplifications that could be made around the callbacks, but if we go that far, it might just be nice to update this lib to use ES6 and promises.

The primary thing I noticed, style wise is this (applied to most of the functions:

_start:  function () { ... whatever }
start: function (offerOptions, next) {
   var self = this;
   self._queue(function(done){
     self._start(offerOptions, next, done);
   });
},

can be simplified to this:

_start:  function () { ... whatever }
start: function (offerOptions, next) {
   this._queue(this._start.bind(this, offerOptions, next));
},

[kenny-house]

Alrighty, I just pushed up the commits moving the _queue calls to use the this._xxxxx.bind(this, args...) version. Great call there, cleans things up quite a bit.

I also moved all of the "wrapper" functions to be above the actual function implementations, that way its a little easier to visually check that the arguments match up.

[fippo]

hrm. @legastero didn't you have queueing in jingle.js?

[kenny-house]

@fippo got me curious... There is indeed an async.queue in jingle-session: https://github.com/otalk/jingle-session/blob/master/index.js#L52-L74

The issue we're seeing (I think) is that while incoming messages from the server are processed in sequence by the queue to completion, calls to the media-session from outside jingle.js (like addStream) are not added to the processing queue but are handled immediately.

Instead of using a queue in jingle-media-session, we could certainly use the same queue that's already on the session, but it would require some updates to how that queue is processing tasks.

[xdumaine]

That's exactly the problem. For that reason, I think it makes sense to have this separate queue here, to separate concerns. But I could be convinced otherwise. Sounds like Fippo doesn't have a strong opinion since he's not using this lib anymore, and I'm getting radio silence from Lance.

[legastero]

Ok, I finally have time to look at this.

1. Queues are great.
2. For things to work, we need all actions to be processed through the same queue. Otherwise you could have a remote and local action happen at the same time and cause problems as we've seen. Queuing only local actions together helps a lot, but doesn't completely solve the problem. The goal is we only ever want one action processing at a time, local or remote.
3. Remote actions are already being queued by the jingle-session module.
4. When mixing local and remote actions into the same queue, we actually want it to be a PriorityQueue where local actions are executed first. Given that the action tie break check doesn't work right now, this change isn't as important yet.

[legastero]

All of that said, I'm fine with these changes. It makes things a lot better already, even if not 100%.

Getting a fully correct solution is going to be a longer undertaking.

[kenny-house]

Thanks for taking a look at things @legastero,

Regarding your second point, I think this PR should at least handle queueing of both remote and local actions in the context of jingle-media-session (since remote actions called from the processing queue in jingle-session will get added to this queue if there are any ongoing actions).