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

Support context.Timeout during Connection initiation. #138

Open
chrisroy87 opened this issue Jul 29, 2022 · 4 comments
Open

Support context.Timeout during Connection initiation. #138

chrisroy87 opened this issue Jul 29, 2022 · 4 comments

Comments

@chrisroy87
Copy link

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
Copy link
Member

Ulexus commented Nov 10, 2022

Oops! That's an unpleasant oversight.

@chrisroy87
Copy link
Author

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

@Ulexus
Copy link
Member

Ulexus commented Nov 11, 2022

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
Copy link
Author

chrisroy87 commented Nov 11, 2022

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.

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