search

englercj / node-esl

FreeSWITCH ESL implementation for Node.js; implements the full Event Socket Library specified in: http://wiki.freeswitch.org/wiki/Esl
http://englercj.github.com/node-esl/
MIT License
170 stars 111 forks source link

Added tests to .api and .bgapi functions on Connection #75

Closed Ricardonacif closed 5 years ago

Ricardonacif commented 5 years ago

Hey,

I was using connection.api in my code and I ran into the following issue: if you call connection.api, then call it again before the callback of the first call is executed, there's a risk of the response of the first call being passed to the second call callback. That happens because it calls apiCallbackQueue.shift() to get the callback to execute, and if the second command executes faster than the first it would get the wrong callback on the array.

I'm not sure if this is by design or a bug, but I could reproduce it on the test case I created (and it doesn't happen on .bgapi. I changed a bit the README but if this is indeed a design choice then I could change the README to be more specific about this.

englercj commented 5 years ago

Thanks for the tests! That behavior is certainly a bug.

I haven't worked in this lib in a long time, do you have a patch that fixes the issue?

Ricardonacif commented 5 years ago

I'll come up with a patch tomorrow!

englercj commented 5 years ago

I'll be out of town for a bit so may take some time before I get back to you on it, but thanks again for looking into it.

Ricardonacif commented 5 years ago

So, I'm inclined to just remove the .api method in a new release. It doesn't make sense on the context of the library. Since it's callback based, we don't stop execution of the code to wait for this synchronous call. And there's no really way to fix it. .bgapi command works because you send a UUID and the response comes with this UUID, so you can relate each call with a response event. But .api command wont return any other param besides OK+. The command is not even documented on the Freeswitch mod_commands anymore ( https://freeswitch.org/confluence/display/FREESWITCH/mod_commands ). Only on old versions of it.

What do you think?

englercj commented 5 years ago

It is still part of mod_event_socket (which is what this module uses to communicate): https://freeswitch.org/confluence/display/FREESWITCH/mod_event_socket

The blocking nature isn't about your app blocking, it is about Freeswitch blocking IIRC. Freeswitch will not process more commands until an api call completes, but a bgapi command is processed in the background and it is available to take more commands in the meantime.

In fact, now that I think about it, it shouldn't be possible for a second api call to complete before the first one because it is a blocking call (on the FSW side), the responses to api calls should always come back in the order they were dispatched.

Ricardonacif commented 5 years ago

So, we can agree you shouldn't be calling multiple .api concurrently right? Because my app was and we got to the bug where it would never run the callback. We changed to .bgapi and its all good now. So should we just specify better on the readme that you shouldn't be calling .api concurrently?

Ricardonacif commented 5 years ago

@englercj

englercj commented 5 years ago

No I think you should be able to make multiple .api calls. They should be serialized on the FSW side and come back down the TCP pipe in order, which is why we shift() the callback because they can't come back out of order (theoretically).

Honestly this sounds like an FSW bug, either in their code or the documentation of the api command. One TCP pipe sending 2 api calls should always return in the same order, is how I read their docs, hence the implementation here.

I'd take it to the FSW forums, and debug an instance of FSW to track it down further.

(sorry for the delayed response, I've been on vacation for the past week)

englercj commented 5 years ago

I think this was possibly caused by #72 which was fixed by #77.

I also was integrating these tests into the v2 rewrite and noticed some things that were wrong in the tests themselves. Once I fixed those things up, it is passing in that branch.

The key takeaway for this issue is that api should run synchronous on FSW, and therefore is garuanteed to come back in order. There is no other way for us to track it because we don't get a job uuid like we do with bgapi. If there are actually coming back across the wire from FSW out of order then I think that is a bug in FSW or a bug in their documentation. If you can confirm they are out-of-order on the wire we can open a new issue to deal with that here.