Fix Windows layer leak when write fails#36728
Conversation
|
LGTM. Nice find. (Although nit, I'm not a fan of named return variables) |
|
@carlfischer1 - for backport consideration |
|
@jhowardmsft Agreed, but in cases like this, named return values are the only safe way to prevent variable shadowing causing |
Codecov Report
@@ Coverage Diff @@
## master #36728 +/- ##
==========================================
- Coverage 35.21% 35.19% -0.02%
==========================================
Files 614 614
Lines 45645 45645
==========================================
- Hits 16073 16067 -6
- Misses 27441 27447 +6
Partials 2131 2131 |
| func writeLayer(layerData io.Reader, home string, id string, parentLayerPaths ...string) (int64, error) { | ||
| err := winio.EnableProcessPrivileges([]string{winio.SeBackupPrivilege, winio.SeRestorePrivilege}) | ||
| func writeLayer(layerData io.Reader, home string, id string, parentLayerPaths ...string) (size int64, err error) { | ||
| err = winio.EnableProcessPrivileges([]string{winio.SeBackupPrivilege, winio.SeRestorePrivilege}) |
There was a problem hiding this comment.
@jhowardmsft would renaming the err output variable to retErr address your concerns?
I know we've had cases where the output variable was overlooked, and naming it retErr instead of plain err makes it a bit more apparent that it's an output variable
There was a problem hiding this comment.
I like that change. Changed to use retErr to prevent confusion.
|
ping @jhowardmsft @johnstep PTAL if the latest version still LGTY |
| return 0, err | ||
| } | ||
| defer func() { | ||
| if err2 := w.Close(); err2 != nil { |
There was a problem hiding this comment.
Does it make sense to rename err2 to err since that no longer conflicts with the return name?
Signed-off-by: Darren Stahl <darst@microsoft.com>
|
LGTM 👍 |
Signed-off-by: Darren Stahl darst@microsoft.com
fixes #31253
- What I did
Fixed a leaked handle that was causing layer deletion to fail when the write of the tar contents failed. This was preventing layer deletion, resulting in a lot of un-removable layers left on disk.
- How I did it
Fixed a leaked handle that occurred when
writeLayerFromTarfailed, as thehcsshim.LayerWriterwas never closed in this case.- How to verify it
Verified locally during stress tests that were causing leaked layers. I've not leaked a layer in days, where before this fix they were leaking regularily.
- Description for the changelog
Fixed a layer leak on Windows.
@johnstep @jhowardmsft