moby / buildkit

concurrent, cache-efficient, and Dockerfile-agnostic builder toolkit
https://github.com/moby/moby/issues/34227
Apache License 2.0
7.85k stars 1.09k forks source link

Always return non-nil contexts #5111

Closed cpuguy83 closed 3 days ago

cpuguy83 commented 3 days ago

This prevents cases where the returned nil context ends up overwriting another context in the caller which can mess up defers.

Ex:

ctx := context.Background()

defer func() {
  cleanup(ctx)
}()

ctx, done, err := leaseutil.WithLease(...)

In this cae when leaseutil.WithLease has an error, before this change the returned context is nil. The solver this can trigger a panic when the context is cancelled because leaseutil will error out and then a prior defer function will call context.WithoutCancel(ctx) triggering the panic because context should never be nil.

leaseutil seems like the only place this was problematic but changed a couple of other places I found to make sure they don't cause problems if the caller changes in the future.

cpuguy83 commented 3 days ago
#36 exporting to GitHub Actions Cache
#36 preparing build cache for export
#36 writing layer sha256:754ce9b4d5cc34ee882f7ad65e9f50b92dd850c6e9ded677107f102aefa5c8b3
#36 writing layer sha256:754ce9b4d5cc34ee882f7ad65e9f50b92dd850c6e9ded677107f102aefa5c8b3 1.5s done
#36 writing layer sha256:795e11f7fbb56a14156b735ce2746a3f43f254eb323a59979caee09483251579
#36 writing layer sha256:795e11f7fbb56a14156b735ce2746a3f43f254eb323a59979caee09483251579 0.1s done
#36 preparing build cache for export 602.8s done
#36 ERROR: maximum timeout reached
------
 > exporting to GitHub Actions Cache: