confusing behavior when a terminal window is closed with `nextstrain view` running about cli HOT 8 CLOSED

Hello @jameshadfield,

I've just tried for 15minutes to reproduce this issue, but I didn't manage to. I've tried on MacOS 10.15.3 , with Docker 19.03.8 and the latest builds of nextstrain.
For the record, I've tried with Terminal.app and iTerm2.app, both with and without tmux.

When closing a terminal, the Docker container is consistently stopped and removed (checked with docker ps -a.

Could you double check if this still happens to you?

Thanks

from cli.

I also can't replicate now.

tom@whunk ~ $ nextstrain version
nextstrain.cli 1.16.2
tom@whunk ~ $ docker version
Client:
 Version:           19.03.6
 API version:       1.40
 Go version:        go1.12.17
 Git commit:        369ce74a3c
 Built:             Fri Feb 28 23:45:43 2020
 OS/Arch:           linux/amd64
 Experimental:      false

Server:
 Engine:
  Version:          19.03.6
  API version:      1.40 (minimum version 1.12)
  Go version:       go1.12.17
  Git commit:       369ce74a3c
  Built:            Wed Feb 19 01:06:16 2020
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.3.3-0ubuntu1~18.04.1
  GitCommit:        
 runc:
  Version:          spec: 1.0.1-dev
  GitCommit:        
 docker-init:
  Version:          0.18.0
  GitCommit:        

It's possible this problem was only occurring prior to 51e93e2, first released in 1.5.0. That commit changed from spawning a child process for the docker invocation to execing into it; the child as an indirect descendant might not have been fully cleaned up by a terminal exit.

from cli.

With the info in this comment from @technicalciso, I was able to reproduce the orphaned container by doing:

1. Observe that no containers are running with docker ps.
2. Run nextstrain build --exec node .
3. Close the terminal it is running in.
4. Observe that the container is still running with docker ps.

However, I still cannot reproduce if I use nextstrain view.

I think more understanding of how nextstrain view is affected could be garnered from bisecting git history.

from cli.

Although I would like to be able to reproduce this first so we can be sure it is fixed, one solution might be running Docker with --init, as I describe in this comment.

from cli.

Ah, as @technicalciso points out, nextstrain view does use --init already (though the underlying runner doesn't).

--init was added in version 1.8.0 before this bug report, but I'm not sure what version James was using at the time of the bug report.

@technicalciso, you said in PR #71 that you could replicate this issue. Could you describe in more detail how you did so, including the commands run, your host OS, and the docker version output?

from cli.

1. open a terminal window (tested with gnome-terminal 3.28.2 and xterm 295.3 with same results)
2. from the cli directory, run bin/nextstrain view /home/me/nextstrain/zika-tutorial/auspice
3. using browser confirm site is working
4. close terminal window (just click the X in the upper right corner)
5. confirm using browser site is still working
6. confirm from another terminal window docker is still running (docker ps)

$ bin/nextstrain version
nextstrain.cli 1.16.2

$ cat /etc/redhat-release
CentOS Linux release 7.7.1908 (Core)

$ docker versionClient:
Version: 1.13.1
API version: 1.26
Package version: docker-1.13.1-109.gitcccb291.el7.centos.x86_64
Go version: go1.10.3
Git commit: cccb291/1.13.1
Built: Tue Mar 3 17:21:24 2020
OS/Arch: linux/amd64

Server:
Version: 1.13.1
API version: 1.26 (minimum version 1.12)
Package version: docker-1.13.1-109.gitcccb291.el7.centos.x86_64
Go version: go1.10.3
Git commit: cccb291/1.13.1
Built: Tue Mar 3 17:21:24 2020
OS/Arch: linux/amd64
Experimental: false

note that i'm using the latest docker which is supported and ships with RHEL (included in the extras repo for centos). when i switch to docker-ce (community edition) at the latest version, which matches what you posted above, i can no longer reproduce.

from cli.

@tsibley just wanted to check in if still worth fixing for only nextstrain view and only on the commercial/RHEL version of docker? otherwise should ticket be closed?

from cli.

Closing this as I suspect it's less of an issue now. It's also worth noting that --tty is now only used when all the standard streams (in, out, err) are attached to a TTY, so a workaround on older systems that might still be affected by this issue would be to redirect one of the streams elsewhere, e.g. nextstrain view … </dev/null.

Happy to re-open if it's still a larger issue.

from cli.