CyCoreSystems / ari

Golang Asterisk REST Interface (ARI) library
Apache License 2.0
180 stars 74 forks source link

Support context.Timeout during Connection initiation. #138

Open chrisroy87 opened 1 year ago

chrisroy87 commented 1 year ago

Presently, the native.Connect doesn't have a mechanism for timeouts. This gives rise to situations where the connection hangs for ever.

Can be re-created easily with wrong address or port during establishing connection.

Asterisk Version: 18 LTS.

cl, err := native.Connect(&native.Options{
        Application:  appName,
        Username:     user,
        Password:     pass,
        URL:          fmt.Sprintf("http://%s:%d/ari", host, port),
        WebsocketURL: fmt.Sprintf("ws://%s:%d/ari/events", host, port),
    })
Ulexus commented 1 year ago

Oops! That's an unpleasant oversight.

chrisroy87 commented 1 year ago

If you want, I can send a PR with the relevant changes.

Ulexus commented 1 year ago

I'd be happy to accept it, thanks. I started looking at it yesterday, and the right way to do it would be to modify the base signatures of the functions to include the context as the first parameter. However, that would break backward compatibility (though that's not necessarily a reason to not do it). A usual method of getting around that is to just create a new function with the Context prefix (e.g. ConnectContext())... that would be fine, too.

BUT, the real problem here is that the underlying websocket dial doesn't use context for its cancellation; it needs to have an explicit timeout declared in its configuration struct. Now, we could derive that from our context's deadline, but that would conflate the retries mechanism, which I don't much like.

So, I think it would generally be better to NOT use context deadlines here at all, but rather extend our Options to include a DialTimeout, which would be used to construct the websocket Dialer options.

chrisroy87 commented 1 year ago

I agree with you there. The way I am doing it now is by creating a *-withContext function as well as expanding the options. This way we can keep the backward compatibility as well as provide a choice for future consumers.