Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context support to ARI client #117

Open
khrt opened this issue Jun 8, 2020 · 6 comments
Open

Add context support to ARI client #117

khrt opened this issue Jun 8, 2020 · 6 comments

Comments

@khrt
Copy link

khrt commented Jun 8, 2020

Would that be possible to add another interface next to ari.Asterisk which supports context?

Also, maybe provide another ARI client with built-in context support, similar to what the package already does with ari/client/native.

@Ulexus
Copy link
Member

Ulexus commented Jun 8, 2020

The ARI client has extensive support for context.Context. We use it for all manner of things, including cleanup items, such as cancelling client subscriptions when the context is closed.

Can you provide more detail in how you with to use it that you currently cannot?

@khrt
Copy link
Author

khrt commented Jun 8, 2020

@Ulexus, for example I'd like to not to send Channel#SendDTMF if context is cancelled.

So I would expect the function signature be something like SendDTMF(ctx context.Context, key *ari.Key, dtmf string, opts *ari.DTMFOptions)

At https://github.com/CyCoreSystems/ari/blob/v4.8.4/client/native/channel.go#L234 we propagate the context further to

r, err = http.NewRequest(method, url, reqBody)

where it then wrapped with the request, like http.NewRequestWithContext

@Ulexus
Copy link
Member

Ulexus commented Jun 8, 2020

Meaning you are wanting to bind a context to a discrete command, not just the entirety of an ARI client connection?

I just realized that I lied: the native client does not use externally-supplied contexts. I so rarely use it directly these days. The ari-proxy client, however, does. Moreover, it allows the use of light-weight derived clients with their own contexts. It's not exactly what you are asking for, but very close.

For instance:

rootCtx := context.Background()
ac, _ := client.New(rootCtx)

timeoutCtx, cancel := context.WithTimeout(rootCtx, time.Second)
ac.New(timeoutCtx).Channel().SendDTMF(key, "1234")

(The two contexts need not be derived; that's just for convenience)

@Ulexus
Copy link
Member

Ulexus commented Jun 8, 2020

That said, it does make some sense to implement some sort of external context control in the native client. I'm not sure how I would want to implement that at this point, though. I don't want to change the basic client interfaces, and since the ARI protocol has two components (the websocket and the discrete http requests), it makes sense to offer separate controls for each, as well.

@khrt
Copy link
Author

khrt commented Jun 9, 2020

One of my uses would also be tracing, like on the options https://github.com/opentracing/opentracing-go provides to you is "context tracing". You propagate a context across all the functions from the very start till the end, allowing to see the whole trace of a single request to you API. For instance: users' request to API -> Business Logic -> Asterisk Send DTMF -> response to API's user.

I indeed understand you cannot simply change the interface as it might be used by someone implemented it with their own HTTP client or anything else.

What I'm asking for is to add either another inteface, let's say ari.AsteriskContext, or extend existing ari.Asterisk with Context counterparts of every function, similar to what happened with database/sql in Go which has Query and QueryContext etc.

@Ulexus
Copy link
Member

Ulexus commented Jun 9, 2020

Yeah, that is a reasonable. This deserves some consideration. I'd rather not just tack on another context-based set when there may be a better way, but if I can't come up with something... that's better than nothing.

As always, PRs are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants