drawing: avoid deadlock when npx is asking for input

[bavovanachte]

Hi @oharboe, thanks for the contribution. Could you provide a bit more input here? Which conditions triggered this in your case and could you provide a .rst that reproduces the issue so that I can include it in CI?

[oharboe]

The problem is when there are unmet dependencies in npx. In this case there is a "do you want to install?" yes/no interactive question. In addition to the DEVNULL, perhaps "--no" should be added to the command line parameters. I prefer an error message to a lockup, as it's easier to debug, so even there should never be any interactive stdinput with "--no", I would prefer to have stdin=DEVNULL

https://docs.npmjs.com/cli/v7/commands/npx

[oharboe]

@bavovanachte Is there anything else I can do?

[bavovanachte]

Sorry for the wait. I tried reproducing the behaviour with npx, and other programs asking for stdin input and didn't see any difference in behaviour, so I'm a bit puzzled by this. I also didn't immediately find any documentation on this. What do you see happening on your end? (which exception is being thrown?)

Regardless, I think adding the -no option to the default wavedrom-cli setting would be a useful addition even if you prefer not to rely on it. I was also considering the timeout option to subprocess.run (user-configurable, with a default of something in the range of 5 mins?)

[oharboe]

Sorry for the wait. I tried reproducing the behaviour with npx, and other programs asking for stdin input and didn't see any difference in behaviour, so I'm a bit puzzled by this. I also didn't immediately find any documentation on this. What do you see happening on your end? (which exception is being thrown?)

I got an error because a module wasn't installed. When stdin is not set to DEVNULL, the process waits for a yes/no.

Regardless, I think adding the -no option to the default wavedrom-cli setting would be a useful addition even if you prefer not to rely on it.

I think adding the -no option is a good idea.

I was also considering the timeout option to subprocess.run (user-configurable, with a default of something in the range of 5 mins?)

I would advice against this. It is much easier to debug an error message than a timeout or deadlock. If the application is asking for input, which it shouldn't, it will fail when stdin is DEVNULL and then the problem can be debugged more easily.

While -no is enough for the case we know about, the DEVNULL will make future surprises easier to debug.

[bavovanachte]

So I tried to reproduce your fix with the following:

wait.sh:

#!/bin/bash
echo "Press any key to continue"
while [ true ] ; do
read -t 3 -n 1
if [ $? = 0 ] ; then
exit ;
else
echo "waiting for the keypress"
fi
done

import subprocess

process = subprocess.run(
    ['bash', 'wait.sh'],
    stdout=subprocess.PIPE, stderr=subprocess.PIPE,
    stdin=subprocess.DEVNULL,
    check=False)

Regardless of whether stdin is set to DEVNULL or not, the process still blocks. Could you point out where my example is flawed?

[bavovanachte]

Bash script copied from https://linuxhint.com/bash_wait_keypress/

[oharboe]

Regardless of whether stdin is set to DEVNULL or not, the process still blocks. Could you point out where my example is flawed?

I believe that it's npx that is checking if the terminal is interactive. If the terminal is not interactive, it doesn't ask.

[bavovanachte]

I believe that it's npx that is checking if the terminal is interactive. If the terminal is not interactive, it doesn't ask.

I've rerun some trials with different npx commands and I can reproduce the behaviour now. So the change is OK for me.

As for the --no option, I'm a bit worried about compatibility with older npx versions. From the docs:

The --no-install option is deprecated, and will be converted to --no.

I've tried an old version myself (that still uses --no-install), and passing the -no option doesn't seem to make it crash, but not sure if that will be the case for every version.

In case you have a smart way around this, I'd still like to add it. If not it might create more problems than it solves and we can merge your PR as is.

[oharboe]

I believe that it's npx that is checking if the terminal is interactive. If the terminal is not interactive, it doesn't ask.

I've rerun some trials with different npx commands and I can reproduce the behaviour now. So the change is OK for me.

As for the --no option, I'm a bit worried about compatibility with older npx versions. From the docs:

The --no-install option is deprecated, and will be converted to --no.

I've tried an old version myself (that still uses --no-install), and passing the -no option doesn't seem to make it crash, but not sure if that will be the case for every version.

In case you have a smart way around this, I'd still like to add it. If not it might create more problems than it solves and we can merge your PR as is.

I wouldnt worry about compatibility with older versions by adding that option. if it doesnt work, seems somewhat unlikely, you will get an error. That should not be hard to track down.

[oharboe]

@bavovanachte Anything more to do here?

[bavovanachte]

Good to go. Thanks for the patience!

[oharboe]

Good to go. Thanks for the patience!

Good job thinking through the edgecases. More speed and less haste does the job.

[bavovanachte]

Tagged as version 3.0.4