The other exec processes will be waiting here. And ctr t ps redis will also be waiting at that time.
/cc @Ace-Tang PTAL

@lifubang the wg.Wait() is used to make sure that the io.Copy has been finished. If we close the io first, it might lost data.

BTW, could you provide the image and /data/start.sh script ? thanks

All in #2624

sleep 100 &
echo ok

Why if -t provided, it can exit quickly, don't need any wait?

@lifubang the wg.Wait() is used to make sure that the io.Copy has been finished. If we close the io first, it might lost data.

BTW, could you provide the image and /data/start.sh script ? thanks

Yes, it should be waiting IO to close by itself. But other exec should not stuck.

Why if -t provided, it can exit quickly, don't need any wait?

for container with -t, io is copy through console, https://github.com/containerd/containerd/blob/master/runtime/v1/linux/proc/exec.go#L215 , seems like when sleep running in the background, io.CopyBuffer(outw, epollConsole, *p) not copy io in this condition, and container task exit. https://github.com/containerd/containerd/blob/master/runtime/v1/shim/service_linux.go#L74

$ time sudo ctr t exec -t --exec-id e1  c1 sh -c 'sleep 10 &'

real	0m0.171s
user	0m0.024s
sys	0m0.028s

in this example, init process is sh -c 'sleep 10 &', it will finish soon, and shim reaper get the SIGCHLD, then console will be shutdown with setExited, for console, io.CopyBuffer(outw, epollConsole, *p) won't wait since it have been shutdown, so wg.Wait also exit.

I have update the code.
Because e.wg.Wait() will hold e.mu.Lock, then p.Pid() and p.SetExited() will stuck.
I think we can remove the lock in p.Delete(), because in stopped state, it can't change to other state.
We should let the lock to other operation.

If hold this lock in Delete(), the command ctr t ps redis and ctr t exec --exec-id e2 redis echo 1 will stuck.

Codecov Report

Merging #2823 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2823   +/-   ##
=======================================
  Coverage   43.79%   43.79%           
=======================================
  Files         100      100           
  Lines       10741    10741           
=======================================
  Hits         4704     4704           
  Misses       5307     5307           
  Partials      730      730

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32aa0cd...951874c. Read the comment docs.

It will cause processExits and checkProcesses stuck.
https://github.com/containerd/containerd/blob/951874c0f68678763d84728059225eb523de63f2/runtime/v1/shim/service.go#L490-L515

@Ace-Tang Thanks your explain for -t flag.

Another solution:
Don't remove lock in func (e *execProcess) Delete(ctx context.Context)
change from:

	e.mu.Lock()
	defer e.mu.Unlock()

TO

	e.mu.Lock()
	e.mu.Unlock()

Don't hold the lock, just check the lock may be more safe.~~

Besides above, and we just hold the lock in execCreatedState, other state just check the lock.
So move the lock to exec_state.go

@thaJeztah this will cause docker container in unhealthy state.

root@dockerdemo:~/nodejs# cat Dockerfile 
FROM node:10
COPY server.js .
COPY start.sh /
HEALTHCHECK --interval=1s --timeout=3s --retries=3 CMD curl -fs http://127.0.0.1/healthcheck || exit 1
CMD node server.js

root@dockerdemo:~/nodejs# cat server.js 
const Server = require('http').Server;
const server = new Server();
server.on('request', (req, resp)=>{
   resp.end('hello');
})
server.listen(80, ()=>{
  console.log('running at :80')
})

root@dockerdemo:~/nodejs# cat start.sh 
#!/bin/bash
sleep 100 &
echo ok

docker build -t node:test .

In this time, it is healty:

root@dockerdemo:~/nodejs# docker run -dit --name nodedemo node:test
b14aa87140bedc8353a3de1576acbbb8453a531b565e9171e74913fd6fe8951d

root@dockerdemo:~# docker ps
CONTAINER ID        IMAGE                                   COMMAND                  CREATED             STATUS                    PORTS               NAMES
b14aa87140be        node:test                               "/bin/sh -c 'node se…"   About an hour ago   Up 47 seconds (healthy)                       nodedemo

If I run:

root@dockerdemo:~/nodejs# docker exec nodedemo /start.sh
ok

It will wait 100s. After about 5 seconds, the container will become unhealthy:

oot@dockerdemo:~# docker ps
CONTAINER ID        IMAGE                                   COMMAND                  CREATED             STATUS                          PORTS               NAMES
b14aa87140be        node:test                               "/bin/sh -c 'node se…"   About an hour ago   Up About a minute (unhealthy)                       nodedemo

After 100s, it will become healthy state again.

PTAL

Yes, we can hold the lock in func (e *execProcess) Delete(ctx context.Context), I think we can let waiting IO completed parallelly. Just like in shell without container. Does it make sense?

With the last commit:

1. ctr t exec --exec-id e1 redis sh -c '/data/start.sh' will wait IO finished. This is reasonable.;
2. ctr t exec --exec-id e2 redis echo 1 will not be stucked anymore when the first command is in running;
3. ctr t ps redis will not be stucked anymore;
4. docker exec redis echo 1 will not be stucked anymore when running together with docker exec redis sh -c /data/start.sh.

For /data/start.sh content:

sleep 100 &
echo ok

I would like to explain how exec without terminal hang if child processes forked from exec process are still alive.

Shim uses runc to create process in container. For the non-terminal case, Shim creates os.Pipe() for the runc process. When runc process runs the real command provided by user, the real command process will hold the same file descriptor created by os.Pipe(). For example, the real command is sh -c "sleep 10 & echo ok".

          /---------------------+
         /                      |
runc -- sh -- sleep 10 &    echo ok
 |       |        |             |
 |       |        |            /
  \      |        |          /
      pipe: [inode number]

The runc, sh and echo will finish and exit before the sleep 10 background job.
They will be reaped by Shim then Shim set the exec process to exited.

However, copyPipes is working and waits for the process to exit. The sleep 10 & background job still hold the writer-side pipe open. That is why the Delete wait for the background job.

I think that it is reasonable. If the background job can output the data, like sleep 10 && date, the data should be show in the pipe. My 2 cents

@lifubang if you can provide the use case, it will be helpful. Thanks

With the last commit:

1. ctr t exec --exec-id e1 redis sh -c '/data/start.sh' will wait IO finished. This is reasonable.;

@fuweid yes, this is reasonable.
But other case is not reasonable. The use case is in
#2823 (comment)
#2823 (comment)

For this patch, we can use a Coarse-grained lock, so don't need modify current lock.
For #2826 , it uses fine-grained locking, we consider different state.

As #2826 is merged, so this can be closed. Thanks all the reviewers.