Put context.Context arguments on almost everything #431

novas0x2a · 2018-03-20T23:12:27Z

This is a pretty huge change, and it breaks api, so consider this first steps :)

After this change:

Network IO paths should react to cancels now.
File IO paths generally still won't.
SystemContext objects have been renamed to sys to leave ctx available for the stdlib context objects.

mtrmac · 2018-03-22T13:34:50Z

vendor.conf

@@ -1,3 +1,5 @@
+github.com/containers/image


One-minute look, will review in detail later:

^^ This seems unexpected.

Overall having a context.Context in most places is definitely valuable.

I’m not sure about passing types.SystemContext and context.Context by a single parameter, notably because SystemContext currently intentionally violates the “Do not store Contexts inside a struct type” rule. Combining them may still be the right thing to do, I’ll need to take a deeper look.

Just replying to the first point; the first line of the trash vendor.conf identifies the package root (e.g.); trash gives me a warning without it (presumably because I use a multiple-GOPATH and it can't identify the canonical parent on its own).

Ah, I had no idea that was a thing. Thanks.

mtrmac · 2018-03-27T14:57:11Z

that's slightly gauche to pass a nil to something that takes a context.Context, but in this case (because the type system can't catch it) I've made it so passing a nil to GetSystemContext behaves the way passing a nil as a SystemContext used to.

That’s not sufficient; if anything actually tries to use the context.Context as a context.Context (e.g. calling Done() or WithCancel(), it will crash on nil.

mtrmac

Overall, I don’t think transferring types.SystemContext as a Value in context.Context makes sense; the two should remain separate.

Looking at how copy.Image uses separate types.SystemContext values for the source and destination (copy.Options.{SourceCtx,DestinationCtx}), it’s clear that the two are not semantically close enough: types.SystemContext is an options storage mechanism, which is propagated along with the relevant data structures; context.Context is a control flow management mechanism, which is propagated along the call stacks and goroutine creations. A single value can’t cleanly be both.
As mentioned earlier, the way SystemContext is stored in ImageSource/ImageDestination implementations violates the context.Context rules (though, arguably, so does the cancelation behavior of http.Request/http.Response).
(types.SystemContext is commonly nil for programs which don’t need non-default behavior, and supporting that for context.Context values is not cleanly possible, nor even desirable—linters which validate the context.Context usage rules should warn about using nil.)

So, I think the two parameters should stay separate, like the current docker.CheckAuth.

We might consider renaming SystemContext to make the two easier to distinguish (CIOptions?), but that’s really a separate discussion and probably a separate PR, if we should do this at all.

Cc: @runcom if he disagrees.

If this PR adds the context.Context parameters, they should actually be used. This is most important for the blocking <- channel operations.

Similarly, though perhaps less urgently, I think many of the local-filesystem operations should be cancelable if they work on layers or whole images; copying a multi-gigabyte image may well take long enough that the caller may realize in the middle that the operation is not needed after all. AFAICT having a cancelable io.Copy helper would make this reasonably easy. OTOH the local disks tend to still be reasonably fast and the most important local storage mechanisms (containers-storage:) seem to not have implemented cancelation/interruptible operations so far, so this is not completely blocking for me; adding FIXME notes to the relevant places would be nice, though.

https://golang.org/pkg/context/#Background documents context.Background as “typically used by…tests”; if we are not actually trying to test cancelation, that’s what we should do. context.TODO is pretty much equivalent to context.Background, and exists primarily to make missing handling easy to find; cluttering the results with all our tests would not be helpful.

mtrmac · 2018-03-27T14:19:31Z

copy/copy.go

@@ -495,9 +496,9 @@ type diffIDResult struct {

 // copyLayer copies a layer with srcInfo (with known Digest and possibly known Size) in src to dest, perhaps compressing it if canCompress,
 // and returns a complete blobInfo of the copied layer, and a value for LayerDiffIDs if diffIDIsNeeded
-func (ic *imageCopier) copyLayer(srcInfo types.BlobInfo) (types.BlobInfo, digest.Digest, error) {
+func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo) (types.BlobInfo, digest.Digest, error) {


The blocking

diffIDResult = <-diffIDChan

needs to abort if ctx is canceled.

mtrmac · 2018-03-27T14:39:49Z

copy/copy.go

@@ -548,7 +549,7 @@ func (ic *imageCopier) copyLayer(srcInfo types.BlobInfo) (types.BlobInfo, digest
 // it copies a blob with srcInfo (with known Digest and possibly known Size) from srcStream to dest,
 // perhaps compressing the stream if canCompress,
 // and returns a complete blobInfo of the copied blob and perhaps a <-chan diffIDResult if diffIDIsNeeded, to be read by the caller.
-func (ic *imageCopier) copyLayerFromStream(srcStream io.Reader, srcInfo types.BlobInfo,
+func (ic *imageCopier) copyLayerFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo,


(Non-blocking: It would be nice to have diffIDComputationGoroutine reliably terminate if ctx is canceled.

It does terminate, ultimately due to the defer…pipeWriter.CloseWithError, but that’s a bit convoluted.

E.g. if a cancelable io.Copy helper were created, using it in the goroutine would be nice.)

mtrmac · 2018-03-27T14:43:41Z

copy/copy.go

@@ -607,7 +608,7 @@ func computeDiffID(stream io.Reader, decompressor compression.DecompressorFunc)
 // perhaps sending a copy to an io.Writer if getOriginalLayerCopyWriter != nil,
 // perhaps compressing it if canCompress,
 // and returns a complete blobInfo of the copied blob.
-func (c *copier) copyBlobFromStream(srcStream io.Reader, srcInfo types.BlobInfo,
+func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo,


(Non-blocking: It would be nice to have compressGoroutine reliably terminate if ctx is canceled, just like diffIDComputationGoroutine.

Same for the

_, err := io.Copy(ioutil.Discard, originalLayerReader)

In all these cases, having the ctx cancelation abort future reads from the Readers should be sufficient, but it’s convoluted.)

mtrmac · 2018-03-27T14:58:14Z

doc.go

@@ -13,12 +13,13 @@
 // 		if err != nil {
 // 			panic(err)
 // 		}
-// 		img, err := ref.NewImage(nil)
+//      ctx := context.Background()
+// 		img, err := ref.NewImage(ctx)


(Nit: The alignment is inconsistent.)

mtrmac · 2018-03-27T15:01:31Z

docker/archive/src.go

@@ -13,7 +14,7 @@ type archiveImageSource struct {

 // newImageSource returns a types.ImageSource for the specified image reference.
 // The caller must call .Close() on the returned ImageSource.
-func newImageSource(ctx *types.SystemContext, ref archiveReference) (types.ImageSource, error) {
+func newImageSource(ctx context.Context, ref archiveReference) (types.ImageSource, error) {


This really should be actually cancelable, in the tarfile.NewSourceFromFile→NewSourceFromStream:

_, err := io.Copy(tarCopyFile, inputStream)

part at least, which, although does not do anything over the network, may be copying many gigabytes of data and take several seconds.

mtrmac · 2018-03-27T17:18:21Z

ostree/ostree_dest.go

@@ -389,7 +390,7 @@ func (d *ostreeImageDestination) PutSignatures(signatures [][]byte) error {
 	return nil
 }

-func (d *ostreeImageDestination) Commit() error {
+func (d *ostreeImageDestination) Commit(ctx context.Context) error {


importBlob, fixFiles, generateTarSplitMetadata, and maybe more, can handle many-gigabyte files or directory structures and should be cancelable.

mtrmac · 2018-03-27T17:21:17Z

ostree/ostree_src.go

@@ -256,13 +256,13 @@ func (s *ostreeImageSource) readSingleFile(commit, path string) (io.ReadCloser,
 }

 // GetBlob returns a stream for the specified blob, and the blob's size.
-func (s *ostreeImageSource) GetBlob(info types.BlobInfo) (io.ReadCloser, int64, error) {
+func (s *ostreeImageSource) GetBlob(ctx context.Context, info types.BlobInfo) (io.ReadCloser, int64, error) {


The goroutine should probably be canceled by ctx.

mtrmac · 2018-03-27T17:32:19Z

storage/storage_image.go

@@ -284,7 +284,7 @@ func (s storageImageDestination) ShouldCompressLayers() bool {

 // PutBlob stores a layer or data blob in our temporary directory, checking that any information
 // in the blobinfo matches the incoming data.
-func (s *storageImageDestination) PutBlob(stream io.Reader, blobinfo types.BlobInfo) (types.BlobInfo, error) {
+func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo) (types.BlobInfo, error) {


The

_, err = io.Copy(diffID.Hash(), decompressed)

can copy many gigabytes of data and should be cancelable.

mtrmac · 2018-03-27T17:41:59Z

storage/storage_image.go

@@ -460,7 +460,7 @@ func (s *storageImageDestination) getConfigBlob(info types.BlobInfo) ([]byte, er
 	return nil, errors.New("blob not found")
 }

-func (s *storageImageDestination) Commit() error {
+func (s *storageImageDestination) Commit(ctx context.Context) error {


The

layer, _, err := s.imageRef.transport.store.PutLayer(id, lastLayer, nil, "", false, diff)

line can copy many gigabytes of data, and should be cancelable. (I’m not sure that can be done right now.)

mtrmac · 2018-03-27T17:44:28Z

tarball/tarball_src.go

@@ -34,7 +34,7 @@ type tarballImageSource struct {
 	manifest   []byte
 }

-func (r *tarballReference) NewImageSource(ctx *types.SystemContext) (types.ImageSource, error) {
+func (r *tarballReference) NewImageSource(ctx context.Context) (types.ImageSource, error) {


The

n, err := io.Copy(ioutil.Discard, reader)

can copy many gigabytes of data and should be cancelable.

novas0x2a · 2018-03-27T22:43:51Z

Hm, I'm willing to follow through with changes like splitting SystemContext and Context apart (which would also resolve the nil problem by making it a compile failure), and I can easily use Background instead of TODO in the tests (generally, I use TODO because those tests are the ones that would catch failures to use ctx in the code, but if you prefer Background, that's fine, too).

I'm probably not willing to take on the task of fixing the whole library re: context behavior, although there's clear value in that, my PR was basically me scratching my itch along the code paths I needed, and then propagating those changes as needed :)

Does that work for you?

mtrmac · 2018-03-27T22:51:58Z

I’m fine with not handling the local FS copies; in the worst case I can add the FIXMEs in there myself.

I’d insist on having the two cases of blocking on a channel handled; that should be roughly

select {
case <-ctx.Done():
	return ctx.Err()
case theVariable <- theChannel:
}

but I guess I can trade that: if you are willing to split the SystemContext and Context, I’ll add the FIXMEs and the channel aborts — and the skopeo updates needed so that tests pass :). Does that make sense?

(generally, I use TODO because those tests are the ones that would catch failures to use ctx in the code, but if you prefer Background, that's fine, too)

I’m not sure what you mean; looking at the implementation, TODO and Background are instances of the same object that differ only in their String() return value. How does using TODO help catch failures to use ctx in the code?

novas0x2a · 2018-03-27T23:03:15Z

Yeah, SGTM. It'll probably take me a few days, but I'll get to it.

I’m not sure what you mean; looking at the implementation, TODO and Background are instances of the same object that differ only in their String() return value. How does using TODO help catch failures to use ctx in the code?

Ah, sorry, I didn't mean the TODOs would, I meant that that they're real TODOs in the sense of indicating the need for future work; the tests they're in would be the things meant to catch failures to use the context. The next step on those TODOs might be to have a test-specific context in the suite that makes sure it cancels the context after each test, so you can't leak threads during the test (for example). But, either way, grepping for context.Background in the tests is just as easy as context.TODO; context.TODO is more for real code to indicate a booby-trap for people debugging something that landed there :)

- Network IO paths should react to cancels now. - File IO paths generally still won't. - `SystemContext` objects have been renamed to `sys` to leave `ctx` available for the stdlib context objects. Signed-off-by: Mike Lundy <mike@fluffypenguin.org>

novas0x2a · 2018-04-07T11:46:39Z

I think I've handled all the comments now, except for the file io paths. How's it look?

mtrmac · 2018-04-09T19:59:08Z

@runcom RFC, please at least ACK the general direction and types.go.

mtrmac · 2018-04-09T21:06:59Z

👍 Thanks!

I have prepared a skopeo PR at containers/skopeo#493 .

Note to self: also check a few of the newImage* for unnecessary parameters.

runcom · 2018-04-10T17:03:47Z

LGTM (just checked types/types.go, it's awesome!)

runcom · 2018-04-10T17:04:14Z

skopeo's PR passing, so merging here

Update for API changes in containers/image#431

novas0x2a force-pushed the context-everywhere branch 2 times, most recently from 625295f to adcbd74 Compare March 21, 2018 20:37

mtrmac reviewed Mar 22, 2018

View reviewed changes

mtrmac mentioned this pull request Mar 22, 2018

docker: tarfile: improve auto-decompression handling #427

Closed

mtrmac reviewed Mar 27, 2018

View reviewed changes

novas0x2a force-pushed the context-everywhere branch from adcbd74 to 369c442 Compare April 7, 2018 11:38

mtrmac mentioned this pull request Apr 9, 2018

Update for API changes in containers/image#431 containers/skopeo#493

Merged

mtrmac mentioned this pull request Apr 10, 2018

context.Context cleanups #440

Merged

runcom merged commit e5b7a01 into containers:master Apr 10, 2018

mtrmac added a commit to containers/skopeo that referenced this pull request Apr 10, 2018

Merge pull request #493 from mtrmac/context-everywhere

e626fca

Update for API changes in containers/image#431

novas0x2a deleted the context-everywhere branch April 10, 2018 17:50

mtrmac mentioned this pull request May 24, 2018

WIP types: use context.Context for public API #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put context.Context arguments on almost everything #431

Put context.Context arguments on almost everything #431

novas0x2a commented Mar 20, 2018 •

edited

Loading

mtrmac Mar 22, 2018 •

edited

Loading

novas0x2a Mar 22, 2018

mtrmac Mar 22, 2018

mtrmac commented Mar 27, 2018

mtrmac left a comment

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

mtrmac Mar 27, 2018

novas0x2a commented Mar 27, 2018

mtrmac commented Mar 27, 2018

novas0x2a commented Mar 27, 2018 •

edited

Loading

novas0x2a commented Apr 7, 2018

mtrmac commented Apr 9, 2018

mtrmac commented Apr 9, 2018 •

edited by runcom

Loading

runcom commented Apr 10, 2018 •

edited

Loading

runcom commented Apr 10, 2018

Put context.Context arguments on almost everything #431

Put context.Context arguments on almost everything #431

Conversation

novas0x2a commented Mar 20, 2018 • edited Loading

mtrmac Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Mar 27, 2018

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novas0x2a commented Mar 27, 2018

mtrmac commented Mar 27, 2018

novas0x2a commented Mar 27, 2018 • edited Loading

novas0x2a commented Apr 7, 2018

mtrmac commented Apr 9, 2018

mtrmac commented Apr 9, 2018 • edited by runcom Loading

runcom commented Apr 10, 2018 • edited Loading

runcom commented Apr 10, 2018

novas0x2a commented Mar 20, 2018 •

edited

Loading

mtrmac Mar 22, 2018 •

edited

Loading

novas0x2a commented Mar 27, 2018 •

edited

Loading

mtrmac commented Apr 9, 2018 •

edited by runcom

Loading

runcom commented Apr 10, 2018 •

edited

Loading