Skip to content
This repository has been archived by the owner on Nov 23, 2019. It is now read-only.

Make streaming operations cancellable. #35

Merged
merged 3 commits into from
Feb 2, 2016

Conversation

calavera
Copy link
Contributor

This change adds a new context argument to all streaming operations to make them cancellable.

Fixes #4.

Signed-off-by: David Calavera david.calavera@gmail.com

@@ -32,7 +34,7 @@ func (cli *Client) ImageBuild(options types.ImageBuildOptions) (types.ImageBuild
headers.Add("X-Registry-Config", base64.URLEncoding.EncodeToString(buf))
headers.Set("Content-Type", "application/tar")

serverResp, err := cli.postRaw("/build", query, options.Context, headers)
serverResp, err := cli.postRaw(context.Background(), "/build", query, options.Context, headers)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my education, why not pass current context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think this function should be allowed to pass a context too.

@calavera
Copy link
Contributor Author

I think we should have several examples on how to use the contexts before merging this in. @mathpl, I really appreciate some help with that if you're up to contribute.

@calavera calavera added this to the Client v1.0.0 milestone Jan 20, 2016
@mathpl
Copy link

mathpl commented Jan 25, 2016

I'd be happy to contribute. Where would you want these, doc.go or actual code under an example directory?

@calavera
Copy link
Contributor Author

@mathpl either sounds good to me. I think the advantage of having them in a doc.go or _test.go as func Example would be that we can show them in godoc.org.

@calavera
Copy link
Contributor Author

I added an example of canceling logs stream. I think this is ready to merge, we can add more examples later.

@calavera calavera force-pushed the cancellable_ops branch 5 times, most recently from 3efa4d4 to 564e6f0 Compare January 29, 2016 19:19
testHookContextDoneBeforeHeaders()
cancel()
// Clean up after the goroutine calling client.Do:
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for spawning a new goroutine here? I would expect Do not to return until the cancellation is acknowledged and the response body is closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calavera calavera force-pushed the cancellable_ops branch 3 times, most recently from 0896856 to 2633c31 Compare February 1, 2016 18:18
@aaronlehmann
Copy link
Contributor

LGTM

ContainerStart(containerID string) error
ContainerStop(containerID string, timeout int) error
ContainerTop(containerID string, arguments []string) (types.ContainerProcessList, error)
ContainerUnpause(containerID string) error
ContainerUpdate(containerID string, updateConfig container.UpdateConfig) error
ContainerWait(containerID string) (int, error)
CopyFromContainer(containerID, srcPath string) (io.ReadCloser, types.ContainerPathStat, error)
CopyFromContainer(ctx context.Context, containerID, srcPath string) (io.ReadCloser, types.ContainerPathStat, error)
CopyToContainer(options types.CopyToContainerOptions) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this non-cancelable?

Signed-off-by: David Calavera <david.calavera@gmail.com>
Use an interface rather than a direct struct.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera
Copy link
Contributor Author

calavera commented Feb 1, 2016

@LK4D4 I've made those two operations cancelable too. Thanks for checking.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@LK4D4
Copy link
Contributor

LK4D4 commented Feb 2, 2016

LGTM

LK4D4 added a commit that referenced this pull request Feb 2, 2016
Make streaming operations cancellable.
@LK4D4 LK4D4 merged commit be6e1ef into docker:master Feb 2, 2016
@calavera calavera deleted the cancellable_ops branch February 2, 2016 17:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants