Nested shells should not be login (#5247)

[dlukes]

Fixes #5247. Opened in response to #4112 (comment) under #4112.

[blink1073]

Nice, thanks!

[blink1073]

@kevin-bates, you had marked this as 6.1 but I think because of the behavior change it should be 6.2.

[kevin-bates]

@blink1073 - this change fixes the regression that the previous change introduced in 6.0.

With this change, typical notebook users (those starting notebook from a shell) will have terminals that retain the current environment (i.e., not using a login and equal to the pre-6.0 behavior), while those users gaining access to notebook servers launched from things like hub or running as a daemon, will launch terminals using a login shell (which addresses the intent of the 6.0 change). As a result, I view this more as bug fix than anything else and why it should be targetted for 6.1.

[blink1073]

Cool, sounds good, merging!

[kevin-bates]

Hmm - I was wanting to try to confirm proper behavior here before merging.

If I launch notebook as a cron job (seemed to be the easiest way w/o Hub integration), I see the trimmed environment and SHLVL == 2! This implies the SHLVL approach is not adequate and we've essentially reverted the reason for @dmikushin's change in the first place.

@dlukes - in what scenarios do you find SHLVL == 0?

[kevin-bates]

If I prefix by 'service command' with SHLVL=0, then this change does the right thing. But I'm not certain using cron for this is a sufficient way to emulate the cloud-based deployment @dmikushin is trying to address.

@dmikushin - would that behavior be sufficient? This doesn't seem sufficient in the Hub use case to me.

@minrk, @Zsailer, @blink1073 - do any of you have additional ideas here?

[kevin-bates]

One "hook" I find in my environments (RHEL and mac) is that env TERM is present in login (or sub) shells, while it is not present when launched from cron. Here's the env from cron (RHEL. mac's is the same sans the XDG_ vars) - which I believe resembles @dmikushin's...

XDG_SESSION_ID=2794
SHELL=/bin/sh
USER=root
PATH=/usr/bin:/bin
PWD=/root
LANG=en_US.UTF-8
SHLVL=1
HOME=/root
LOGNAME=root
XDG_RUNTIME_DIR=/run/user/0
_=/usr/bin/env

As a result, using this as the criteria for creating a login shell seems to work...

if os.name != 'nt' and not os.environ.get("TERM"):

In fact, using TERM is the only thing I see that can be leveraged to make this determination.

Thoughts?

[dlukes]

in what scenarios do you find SHLVL == 0?

I run a JupyterHub instance on an Ubuntu 18.04 server and when I log in (i.e. a spawn a single-user server with SystemdSpawner), launch a notebook and run import os; os.environ["SHLVL"] or !echo $SHLVL, I get '0' / 0. When I launch a terminal and echo $SHLVL, I get 1. So even with this patch, the shell inside the terminal should be started as login AFAICS.

Binder seems to be configured the same way, if you want to try it out, I get the same behavior.

Of course, I can imagine this may vary across *nix flavors and specific setups, particularly across spawners other than SystemdSpawner? If so, that's unfortunate, but I would argue that JupyterHub admins are typically more technically sophisticated than someone who runs the Jupyter notebook locally, and so they're in a better position to work around the issue. Contrast this with an individual user with a local install whose virtualenv gets overridden out of the blue.

In fact, using TERM is the only thing I see that can be leveraged to make this determination.

Sorry, I have no idea if that would be more or less reliable than SHLVL across different *nix flavors... Maybe someone else?

FWIW, running !echo $TERM in a Binder notebook prints xterm-color, and echo $TERM in a Binder terminal prints xterm. The same goes for the JupyterHub instance I run.

[kevin-bates]

Thanks for the helpful response @dlukes. So clearly your use-case is covered by SHLVL - awesome.

I'm learning that some shells don't use SHLVL and I suspect platforms that do use it may vary on whether it starts at 0. As a result, I'm beginning to think we might be best off with an either-or strategy here:

if os.name != 'nt' and (int(os.environ.get("SHLVL", 0)) < 1 or not os.environ.get("TERM")):

Unless @dmikushin can confirm that using the SHLVL < 1 check works and that gives us (me?) the necessary confidence to keep this change, I'm inclined to propose the additional clause.

[blink1073]

It looks like not all shells implement SHLVL, such as ksh and csh: https://docstore.mik.ua/orelly/unix3/upt/ch04_12.htm

It looks like checking for TERM is not a reliable way to tell if you're running a CRON job because that variable might be set through some other means. Here were some suggested ways to detect when running on CRON: https://unix.stackexchange.com/a/46801

[dlukes]

I just want to stress that we shouldn't lose sight of the fact that in the basic use case (= local installation), a non-login shell is the correct default. The typical local user will launch the notebook from a shell which presumably already has an appropriately configured environment, so it doesn't make sense to run the risk of breaking it (as can happen on macOS, cf. #5247) by running a login shell.

Providing an escape hatch for more advanced use cases which don't have a login shell higher up the process tree, and therefore may lead to an incomplete environment, is definitely a good thing, but I think it should:

1. be on a best effort basis -- the heuristics detecting the need for a login shell can be incomplete, requiring the advanced user to add their own workaround in some cases, or contribute a PR
2. err on the side of caution so as not to break the basic use case above -- the heuristics should ideally have a 0 false positive rate, but false negatives are acceptable

I thought checking SHLVL was relatively safe in this respect, since in the basic use case, the notebook is launched from an existing (presumably) well-configured shell, as noted above. However, given that not all shells implement it (thanks for researching that, @kevin-bates and @blink1073!), it could lead to pretty glaring false positives for ksh and csh users. Though I'm not sure how many people that is, since ksh and csh are literally the only two shells of all the ones I could think of off the top of my head that I couldn't easily install and test, because they don't seem to be in my OS's package repos :)

At first glance, checking TERM seems relatively safe too -- in the basic use case, that shell that the notebook is launched from has to be running inside a terminal. As @blink1073 points out, it can in theory be (manually) set even in other cases, e.g. under cron, but that would lead to a false negative, which doesn't worry me too much, as per the discussion above. However, it can also be unset in that basic use case, which would lead to a false positive. I don't know how often people unset TERM in their .bashrc / .zshrc / config.fish (it certainly sounds like a bad idea), but there's nothing stopping them.

In summary, this is (unsurprisingly) a problem with any heuristic based on checking environment variables -- they can be overridden in every which way.

So what about checking sys.stdout.isatty() instead? That should reliably be True whenever the notebook is launched from a shell connected to a terminal (the basic use case). It's also False when running under cron or when launched by systemd -- I mean obviously it is, but of course I checked :) Can anyone think of a counterargument exposing why this might not be a good idea?

[dlukes]

This may be no longer relevant if the SHLVL heuristic is discarded, but in case it isn't:

If I launch notebook as a cron job (seemed to be the easiest way w/o Hub integration), I see the trimmed environment and SHLVL == 2!

The cron environment itself doesn't set SHLVL, which you can confirm by putting * * * * * /usr/bin/env >"$HOME/cron-env" in your crontab, waiting a bit, and inspecting ~/cron-env.

If you run a shell script from cron that launches a jupyter notebook server, that shell script will have SHLVL == 1, which the notebook server will inherit. So if you then start a terminal from that notebook server, the shell inside it will have SHLVL == 2.

As you've rightly pointed out, SHLVL can be overridden to 0 in that launcher shell script, which makes everything work as intended. This could be documented as the "official" solution, which admins of advanced configurations can be expected to look up, as opposed to regular users.

At that point though, it might be better to just use a dedicated env var (JUPYTER_...) or config setting, rather than piggybacking on a (combination of) existing one(s). Unlike SHLVL / TERM, one would hope that people won't happen to accidentally override a dedicated env var for a completely unrelated reason.

I suspect platforms that do use it may vary on whether it starts at 0

I may be wrong, but I would tend to think that in most JupyterHub settings, SHLVL won't be set at all when a single-user notebook server is launched, because it won't be launched from a shell :) Though this may vary depending on the spawner used, as I mentioned previously.

[blink1073]

I like the idea of using sys.stdout.isatty() as the default, but allowing for a JUPYTER_SHELL_LOGIN override. I think the "somewhat standard" environment variables have too many edge cases, as you've pointed out @dlukes.

[dlukes]

Can anyone think of a counterargument exposing why this [i.e. sys.stdout.isatty()] might not be a good idea?

One potential problem I can think of is if a distribution provides a desktop launcher for the notebook which doesn't run it inside a terminal.

Are there downstream packagers out there that do this? If so, it would be unfortunate, because literally the most beginner-friendly way of launching the notebook (= via the GUI) would do the wrong thing.

Arguably though, not running inside a terminal is the wrong way to provide a desktop launcher for the notebook, because it hides potentially useful logging output from the server. Notably, the Anaconda Navigator launcher starts a terminal and runs the notebook server inside it, so that one would work fine.

[blink1073]

@captainsafia @rgbkrk does nteract launch the jupyter server in a system terminal?

[blink1073]

@timkpaine would this affect your custom launching logic? iirc you are launching subprocesses running the jupyter lab command.

[blink1073]

Ah, if the server is run in a docker container that is launched without the -i or -t options, this would probably return false.

[blink1073]

I'm testing the docker question now.

[dlukes]

I tried distro-specific ways of installing the notebook on Ubuntu, Debian, Fedora and Solus. The only package I found that provides a desktop launcher is this snap, and it opens a terminal, so that's fine.

Ah, if the server is run in a docker container that is launched without the -i or -t options, this would probably return false.

Good point! I'm not sure what should be considered "correct" behavior in a Docker context, but I can see how having it be dependent on Docker command line switches is somewhat unfortunate.

OTOH, people don't tend to muck about too much with shell configuration inside Docker containers, so maybe the login/non-login distinction doesn't matter as much there?

The main argument against a login shell by default is that it might override manual changes to the environment. As in, stuff that the user literally types into the shell prior to launching the notebook, as opposed to stuff that lives in any (system-wide or user-specific) init files.

In the Python world, a prime (and maybe the only?) example of that is virtualenvs (that's the bug that bit me in #5247). But running the notebook inside a virtualenv inside Docker sounds like belt and suspenders to me. IDK -- do people do that? If not, maybe it's not worth worrying too much about.

[blink1073]

Pulling down the jupyter docker image took longer than I thought, I'll have to experiment more later today.

[dlukes]

I thought of doing something like the following to walk up the process tree and see if there's a login shell (= a - is prepended to the program name) among the ancestors:

import os
from psutil import Process

def is_child_of_login_shell(pid):
    for parent in Process(pid).parents():
        if parent.cmdline()[0].startswith("-"):
            return True
    return False

print(is_child_of_login_shell(os.getpid()))

But that doesn't work in a local GUI, because GUIs typically don't go through a login shell and use a custom mechanism to set up the environment.

[kevin-bates]

I'll catch up on the responses later today (thank you for the efforts - they're much appreciated!), but I wanted to clarify this comment from Steve...

It looks like checking for TERM is not a reliable way to tell if you're running a CRON job

I was merely using CRON as a means of launching a notebook instance in a manner similar to how a service might be launched. I didn't mean to imply that CRON-based launches are something we should try to support. I was just using it to generate one of these minimal configurations and how to work backwards from there in order to solve this.

I just want to stress that we shouldn't lose sight of the fact that in the basic use case (= local installation), a non-login shell is the correct default.

@dlukes - I agree 100% and made comments similar to this in my original questioning of using -l. If we feel SHLVL is adequate I'm okay with just leaving things alone. Given all the overnight responses (thanks again!) I'm starting to think we're spending too much time on this. I'll try to follow-up this afternoon.

[blink1073]

Turns out I'm not feeling well enough to work today, I'll circle back to this.

[kevin-bates]

Sorry to hear you're not feeling well Steve - get better soon and thank you for spending time on this. (I spent some time with the jupyter/scipy-notebook image and didn't see any discernible differences w/ and w/o -it.)

David, I just caught up on your comments - wow - thanks for the great investigation! I too like the idea of leveraging isatty() and was thinking about combing this with Steve's suggestion (which I suspect was Steve's thought as well):

if os.name != 'nt' and int(os.environ.get("JUPYTER_SHELL_LOGIN", not sys.stdout.isatty()):

We'd document JUPYTER_SHELL_LOGIN to be '0' or '1' to simplify conversion to bool, etc.

Another option would be ... we rollback everything!

It turns out a solution was configurable all along ...

## Supply overrides for terminado. Currently only supports "shell_command".
c.NotebookApp.terminado_settings = {"shell_command": ['/bin/bash', '-l']}

Since the ship has already sailed with 6.0 and the isatty() approach silently addresses both use-cases w/o intervention, it may be preferred. I'd be cool with reverting to the original as well - although it would revert behaviors for those needing a login terminal until they adjust their configurations.

cc: @dmikushin

[blink1073]

I agree with all of the above (except for rolling back), thanks @kevin-bates!

[dlukes]

It turns out a solution was configurable all along ...

Of course! I'd completely forgotten about that, thanks for pointing it out :)

Building on that: on reflection, the custom env var feels a bit inconsistent with how Jupyter is typically configured. Correct me if I'm wrong, but AFAIK, the entire ecosystem systematically uses config files and command line switches for this purpose. There are a few env vars here and there, but a potential JUPYTER_SHELL_LOGIN feels like the odd one out. I'm also not entirely sure where it would fit in the notebook config docs.

Moreover, it seems like c.NotebookApp.terminado_settings already provides the perfect hook for a much more expressive override:

    shell_override = nb_app.terminado_settings.get('shell_command')
    shell = (
        [os.environ.get('SHELL') or default_shell]
        if shell_override is None
        else shell_override
    )
    # When the notebook server is not running in a terminal (e.g. when
    # it's launched by a JupyterHub spawner), it's likely that the user
    # environment hasn't been fully set up. In that case, run a login
    # shell to automatically source /etc/profile and the like, unless
    # the user has specifically set a preferred shell command.
    if os.name != 'nt' and shell_override is None and not sys.stdout.isatty():
        shell.append('-l')

(Here's the commit/diff: dlukes@fad1ce5)

This is also better for edge cases such as someone using a very weird shell which doesn't accept an -l parameter -- as long as they write out the desired command under shell_command, they can be sure Jupyter won't tamper with it, which wouldn't be the case if the condition for appending -l relied purely on checking JUPYTER_SHELL_LOGIN and isatty().

I don't know, just my two cents :) I realize I'm an outsider and my perspective may be fairly limited.

[kevin-bates]

@dlukes - thank you again.

Building on that: on reflection, the custom env var feels a bit inconsistent with how Jupyter is typically configured. Correct me if I'm wrong, but AFAIK, the entire ecosystem systematically uses config files and command line switches for this purpose. There are a few env vars here and there, but a potential JUPYTER_SHELL_LOGIN feels like the odd one out. I'm also not entirely sure where it would fit in the notebook config docs.

You're absolutely correct in that Notebook has tried to avoid the use of env variables - although I think they are much more useful in container landscapes, etc.

I think your latest solution provides all the necessary "options" for navigating this minefield and I'd be okay with that. This is another hybrid approach that, I believe, addresses things in the best way so far.

@blink1073 (once you're feeling better) - what do you think?

[dlukes]

Notebook has tried to avoid the use of env variables - although I think they are much more useful in container landscapes, etc.

That's a very good point :) I just wanted to put that alternative out there, because I guess I have an aesthetic preference for not introducing another configuration option when one already exists that does the job perfectly well. So I was really glad you'd unearthed c.NotebookApp.terminado_settings. But as I've said, my perspective may be fairly limited, and there are perhaps more important points of view to consider here than aesthetics.

[blink1073]

I support dlukes/notebook@fad1ce5 as well, thanks to you both!

[kevin-bates]

@dlukes - could you please create another PR?

[dlukes]

@kevin-bates Sure, happy to! #5588