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

use context.Context for timing out operations #362

Open
jakecoffman opened this issue Mar 19, 2021 · 4 comments
Open

use context.Context for timing out operations #362

jakecoffman opened this issue Mar 19, 2021 · 4 comments

Comments

@jakecoffman
Copy link

Currently there's no way to set a timeout for an operation using this library. Generally this is done in Go using the context package: https://blog.golang.org/context

In a fork I've begun adding context everywhere that needs it. If you are interested in this change upstream let me know and I can put a PR up when it's done, otherwise I'll just use the fork since I need the timeouts.

@Didainius
Copy link
Collaborator

Hello @jakecoffman,
Thanks for looking into it. I wonder what is your approach for cancellation? Are you cancelling tasks or something more?

Would be interesting to see your fork/branch

@jakecoffman
Copy link
Author

Here's an in-progress branch: master...jakecoffman:context

I need to cancel the underlying HTTP request (by using http.NewRequestWithContext), not the command or task itself. That way when my server that makes calls to vcloud needs to go down, if it happened to just launch a long running operation it can cancel and clean things up before it's killed.

Unfortunately by switching to http.NewRequestWithContext we have to pass a Context down from every function that calls it, which is nearly every function in this project.

@lelvisl
Copy link
Contributor

lelvisl commented May 8, 2021

It will be useful for using with tracing.

	vcdclient.Client.Http.Transport = &ochttp.Transport{
		Base:        vcdclient.Client.Http.Transport,
	}

Now, without context, i have separated traces.

@jakecoffman
Copy link
Author

I have a fork here with the context applied if anyone would like to use it before it is available in this repo: https://github.com/wwt/go-vcloud-director

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

3 participants