(bg)api callbacks are not invoked on socket errors

[Lekensteyn]

Consider a long-running daemon with this made-up example:

var conn;
function connectToFS(host, port, password) {
    conn = new esl.Connection(host, port, password, function() {
        console.log('Got FS');
    });
    conn.on('error', function(error) {
        conn = null;
        console.error('Connection failed: ' + error);
        setTimeout(connectToFS, 1000, host, port, password);
    });
}
// ...
function requestCallStatus() {
    return Promise(function(resolve, reject) {
        if (!conn) return reject('FS connection not ready');
        conn.bgapi('show calls', function(evt) {
            var calls_count = parseInt(evt.body);
            resolve(calls_count);
        });
    });
}

If at some point the connection is severed (server restart, network failure), then the following message is visible:

Connection Failed: TypeError: Cannot read property 'write' of null

but the callback (promise) is never called and callbacks are left dangling. While is is possible to add an extra error listener before attempting the query, it is a bit hacky.

Would it be an idea to add a second parameter (error) to the callbacks such that something like this is possible:

    conn.bgapi('show calls', function(evt, error) {
        if (!evt) return reject(error);
        var calls_count = parseInt(evt.body);
        resolve(calls_count);
    });

On the library side, I think that the callback queues (cmdCallbackQueue, apiCallbackQueue) can be flushed as a start, but that does not help with other send() users. So maybe the send() function should receive an (optional) error callback or propagate errors.

What do you think of the stated problem and possible approaches to solve it?

[englercj]

Right now the API is such that only responses are passed to the callback in the form of an event.

Connection errors are handled at the global level on the connection object.

Maybe we can add support for an "Error Event" when there is a connection-level error, but this would be diverging from the ESL spec (http://wiki.freeswitch.org/wiki/Esl) which this library tries to follow as closely as possible. Most likely, you just need to track/reject your promises if there is a connection error. Should be simple to create a Promise wrapper that automatically adds/removes promises to some global list when created/resolved/rejected.

[Lekensteyn]

I don't think that adding finer error events will help in this case (unless you are also automatically unregistering the event after successfully sending the error, but then you probably have to set the "job id" yourself or you won't know which call exactly failed).

The current workaround involves maintaining a mapping from a monotonically increasing counter to the reject function. Before calling bgapi, the function is added. In the callback, it is removed. In the error handler, all callbacks are cleared and invoked.

Additional robustness is obtained by calling conn.connected() to check whether the socket is dead:

function checkFsCon() {
    if (!conn)
        return false;
    if (!conn.connected()) {
        console.log('Connection lost, disconnecting.');
        // NOTE: this relies on 'error' being invoked because send('exit') should fail.
        conn.disconnect();
        return false;
    }
    return conn;
}

[englercj]

The current workaround involves maintaining a mapping from a monotonically increasing counter to the reject function. Before calling bgapi, the function is added. In the callback, it is removed. In the error handler, all callbacks are cleared and invoked.

That is exactly what I suggest, but I would do it by creating a wrapper around Promise that did it automatically to reduce boilerplate.

[Lekensteyn]

Done that, it is a good suggestion. Now I can use bgapi(command, arg).then(...).catch(...) which can also be chained (needed for my new use case) :-)

With this pattern a local error callback is no longer needed. (It needs to be revisited though if the same Connection object is reused via a possible "fix" of #34, otherwise the wrong (dangling) callbacks might invoked later.)

[englercj]

Glad you got a solution for your issue.

Unfortunately I don't work in telephony anymore and don't use this library in any of my projects, so I don't work on it actively anymore. I'm always open to PRs!

[Lekensteyn]

Thank you for your help and time @englercj , it is appreciated. I think that the current workaround is the safest without introducing possibly backwards incompatible issues so will leave it here.