Awaiting driver -> FinishPipe blocks further operations

[allisonkarlitskaya]

I'm not 100% sure about this, but based on my reading of the code, awaiting a "driver" object from a blob download will effectively block all other activity (like starting new blob downloads, for example).

This is caused by the fact that the skopeo proxy protocol doesn't appear to support tagging (ie: multiple in-flight requests) and we take a lock while sending a request and awaiting a reply. That means if we send FinishPipe, we'll end up waiting (with the lock held) until we get a reply, which will block all other requests on the proxy...

That means that the officially documented way of interacting with the proxy (ie: joining the driver wait with the reading of the pipe) will interact poorly with trying to use the proxy from multiple threads (which was enabled by #78). composefs-rs mostly avoids this fate only because we only ever try to await the driver after we finished reading the entire stream (we use the "take" trick there, with the size from the descriptor). I have seen a few cases where everything seems to serialize up (ie: no new layers starting) after a big read finishes, though, and I have a feeling it's related to this...

[jeckersb]

On the skopeo side, things are very much linear / single-threaded as it exists today. The main loop is just reading one request from the control socket, handling the request in its entirety, and sending back the reply. You'd need multiple proxy instances in order to pull multiple streams in parallel. Not ideal.

Thanks for bringing this up though, because it's worth consideration alongside #79.

[allisonkarlitskaya]

containers/composefs-rs#111 very much succeeds at pulling in parallel (extensively) using a single proxy connection...

skopeo itself documents that this should be possible too: https://github.com/containers/skopeo/blob/bb26ea90f9c9c1b183bbc58c2287411c0b0bae71/cmd/skopeo/proxy.go#L48

[allisonkarlitskaya]

Right, so the streaming of layers can be done concurrently but the request channel is indeed very much serial.

// FinishPipe waits for the worker goroutine to finish, and closes the write side of the pipe.
func (h *proxyHandler) FinishPipe(args []any) (replyBuf, error) {
...
	// Wait for the goroutine to complete
	f.wg.Wait()

Removing the await on the driver in the composefs-rs code seems to eliminate the little stalls (and gives a very very small performance increase), so it kinda feels like this is indeed the issue...

Since we check the layer ID checksums on the composefs-rs side, I'm kinda inclined to just never call FinishPipe. Funny:

	// And only now do we close the write half; this forces the client to call this API
	f.w.Close()

... I don't feel forced ;)

[cgwalters]

Agree this is a problem.

Not calling FinishPipe will leak the pipes; not that in practice it really matters if one is only allocating a proxy per image and not persisting it, since it will all get cleaned up when the process exits.

I think a workaround for now though though is just to call FinishPipe at the end for all layer fetches right?

[jeckersb]

Yeah you just gotta be careful to only call FinishPipe on things that you're truly sure is ready to be finished (e.g. you've already emptied the pipe to EOF), otherwise you'll block up the (serial) control channel and nothing will be able to progress anymore.

This kinda ties into #79 too, wherein we want to expose a tweaked API with less footgun potential.

[allisonkarlitskaya]

I think a workaround for now though though is just to call FinishPipe at the end for all layer fetches right?

This would require passing the driver futures out of the layer-fetching tasks.... I think I'd prefer to just leak it, knowing that they're gonna be cleaned up when I close the proxy. We check the uncompressed layer checksum in composefs-rs ourselves when creating the splitstream, anyway.