asterisk / node-ari-client

Node.js client for ARI. This library is best effort with limited support.
Other
250 stars 98 forks source link

ari-client.connect authentication error should be catchable #81

Open TomDmitriev opened 7 years ago

TomDmitriev commented 7 years ago

Here is a very simple use case

try {
    ari.connect(config.url, config.user, config.pass, connErr => {
        if (connErr) {
            logger.log('error', 'caught from callback', connErr);

            return;
        }

        logger.log('info', 'connected');
    }).catch(e => {
        logger.log('error', 'caught from promise', e);
    });
} catch (e) {
    logger.log('error', 'caught from try-catch', e);
}

If the credentials are invalid I get this error message and the program exits.

/home/artyom/WebstormProjects/pbsw-dialer/node_modules/swagger-client/lib/swagger.js:270 throw message; ^ 401 : undefined http://localhost:8088/ari/api-docs/resources.json

I never actually catch the error which means I can't perform a resource cleanup or produce a custom error message or wrap the error or anything like that. I believe all connection errors should be catchable.

samuelg commented 7 years ago

Since the connect is asynchronous, it cannot be caught the way you show above. Node is in a different context by the time an error is returned. Having said that, the callback should return an error object for that case. The problem today is that this is not exposed by the version of swagger-js used and the latest version removed access to metadata we use to build the client dynamically.

One option is to preemptively connect using another http client and catch the error that way before connecting using the swagger-js library but it's far from ideal.

ibc commented 6 years ago

With all my respect, this is not acceptable. If swagger-js is not suitable because it does not allow catching initial WebSocket connection errors, then do not use swagger-js.

A much better alternative: http://chadmcelligott.com/awry/

samuelg commented 6 years ago

I'm afraid I don't have the free time I used to have to work on this. There's a PR up with a work around that needs to be merged in since the code base diverged after it was proposed.

I do agree the way forward is to get rid of swagger-js but again I do not have the resources to do it at the moment.

As always, contributions are welcome. If you find that awry works well for you that's great. Chad does awesome work.