task: don't close() io before cancel()

laurazard · laurazard · commit b27f7daa582f · 2023-06-10T11:35:09.000+01:00
The contract for `cio/io.go/IO` states that a call to `Close()` will always be preceded by a call to `Cancel()` - https://github.com/containerd/containerd/blob/f3a07934b49bf142925a5913e3e19f3528eda0d2/cio/io.go#L59 which isn't being held up here. Furthermore, the call to `Close()` here makes the subsequent `Wait()` moot, and causes issues to consumers (see: moby/moby#45689) It seems from https://github.com/containerd/containerd/blob/f3a07934b49bf142925a5913e3e19f3528eda0d2/task.go#L338 that the `Close()` should be called there, the call removed in this commit is unnecessary/erroneous. We leave the `Close()` call on Windows only since this was introduced in #5974 to address #5621. Signed-off-by: Laura Brehm <laurabrehm@hey.com> (cherry picked from commit 34a93a0) Signed-off-by: Laura Brehm <laurabrehm@hey.com>
diff --git a/task.go b/task.go
@@ -325,7 +325,16 @@ func (t *task) Delete(ctx context.Context, opts ...ProcessDeleteOpts) (*ExitStat
 		return nil, fmt.Errorf("task must be stopped before deletion: %s: %w", status.Status, errdefs.ErrFailedPrecondition)
 	}
 	if t.io != nil {
-		t.io.Close()
+		// io.Wait locks for restored tasks on Windows unless we call
+		// io.Close first (https://github.com/containerd/containerd/issues/5621)
+		// in other cases, preserve the contract and let IO finish before closing
+		if t.client.runtime == fmt.Sprintf("%s.%s", plugin.RuntimePlugin, "windows") {
+			t.io.Close()
+		}
+		// io.Cancel is used to cancel the io goroutine while it is in
+		// fifo-opening state. It does not stop the pipes since these
+		// should be closed on the shim's side, otherwise we might lose
+		// data from the container!
 		t.io.Cancel()
 		t.io.Wait()
 	}
