Clarify what the connection lifecycle should be in the bindings

[aureliar8]

I'm trying to use the podman bindings in a gRPC server that needs to list OCI images present on the host.

My question is: Should I use a single long-lived connection to the podman socket or should I create one for each request ?

On one hand creating a new connection for each request is the most straightforward thing to do:

func (s *Server) ListImage(ctx context.Context, req *pb.ListImageRequest) (*pb.ListImageResponse,error){
    ctx, err := bindings.NewConnection(ctx, s.podmanSocket)
    if err != nil {
        return nil, fmt.Errorf("connecto to podman socket: %w", err)
    }
    images, err = images.List(ctx, nil)
    if err != nil {
        return nil, fmt.Errorf("list images: %w", err)
    }
    return &pb.ListImageResponse{images: images}, nil 
}

But it seems a bit wasteful to recreate a new connection to the socket for every request. Moreover a bindings.Connection contains an http.Client that is created for each call to bindings.NewConnection. And the documentation of http.Client states the following:

The [Client.Transport] typically has internal state (cached TCP connections), so Clients should be reused instead of created as
needed. Clients are safe for concurrent use by multiple goroutines.

But trying to reuse a long-lived bindings.Connection is quite difficult, in part because it is hidden inside a context.Context:

func (s *Server) ListImage(ctx context.Context, req *pb.ListImageRequest) (*pb.ListImageResponse,error){
    images, err = images.List(s.podmanConn, nil) // s.podmanConn is initialized at startup
    if err != nil {
        return nil, fmt.Errorf("list images: %w", err)
    }
    return &pb.ListImageResponse{images: images}, nil 
}

This naive usage is simple, but wrong. Because the request context isn't used to perform the image.List call, and thus it will not react to the request context being cancelled.

At the moment (and unless I'm missing something), the only way to reuse a long-lived connection while still abiding to shorter request context is to do recreate a new context.Context that is basically a merge of the request context and the connection context:

func (s *Server) ListImage(ctx context.Context, req *pb.ListImageRequest) (*pb.ListImageResponse,error){
    ctx, cancel := mergeContext(s.podmanConn, ctx)
    defer cancel()
    images, err = images.List(ctx, nil)
    if err != nil {
        return nil, fmt.Errorf("list images: %w", err)
    }
    return &pb.ListImageResponse{images: images}, nil 
}

// mergeContext return a context that have the values of podmanCtx and which is canceled when either
// podmanCtx or requestCtx is canceled.
func mergeContext(podmanCtx, requestCtx context.Context) (context.Context, context.CancelFunc) {
	ctx, cancel := context.WithCancelCause(podmanCtx)
	stop := context.AfterFunc(requestCtx, func() {
		cancel(context.Cause(requestCtx))
	})
	return ctx, func() {
		stop()
		cancel(context.Canceled)
	}
}

If reusing a long-lived connection is indeed correct, then I'd propose to either:

• Add a helper function to merge a long-lived connection context and a request context

func (s *Server) ListImage(ctx context.Context, req *pb.ListImageRequest) (*pb.ListImageResponse,error){
    ctx, cancel := bindings.MergeCtx(s.podmanConn, ctx) //s.podmanConn is a context.Context here
    defer cancel()
    images, err = images.List(ctx, nil)
    if err != nil {
        return nil, fmt.Errorf("list images: %w", err)
    }
    return &pb.ListImageResponse{images: images}, nil 
}

• Make it possible to add a bindings.Connection to a regular context.Context

func (s *Server) ListImage(ctx context.Context, req *pb.ListImageRequest) (*pb.ListImageResponse,error){
    images, err = images.List(ctx.WithValue(bindings.ClientKey, s.podmanConn) // s.podmanConn is a bindings.Connection here
    // or with a helper
    // images, err = images.List(bindings.WithConnection(ctx, s.podmanConn), s.podmanConn)
    if err != nil {
        return nil, fmt.Errorf("list images: %w", err)
    }
    return &pb.ListImageResponse{images: images}, nil 
}

What do you think ? I'd be happy to make a PR to implement one of those change