This was an issue wherever I tested it. Adding a "wait" seems to fix things. I'm not 100% sure if it is the right fix. I think it is.
Before this patch I found that proc.returncode was none until it called proc.communicate at which point it always returned 0. The previous behavior was to return zero regardless what was clicked.
Hmmm.
returncode is meant to be None until the process has terminated.
communicate()
does call wait()
.
I'm not sure reading from the pipes after the process has already terminated is going to work everywhere. That could cause SIGPIPE
s.
My guess is that this is a WSL bug. Maybe the _eintr_retry_call
subprocess function doesn't trap the exceptions thrown. (ie EWOULDBLOCK
/ WSAEWOULDBLOCK
on win32 - maybe they don't translate that properly)
Can we detect WSL and only wait prematurely when really needed?
Replying to Antoine Martin:
Hmmm. returncode is meant to be None until the process has terminated.
communicate()
does callwait()
. I'm not sure reading from the pipes after the process has already terminated is going to work everywhere. That could causeSIGPIPE
s.My guess is that this is a WSL bug. Maybe the
_eintr_retry_call
subprocess function doesn't trap the exceptions thrown. (ieEWOULDBLOCK
/WSAEWOULDBLOCK
on win32 - maybe they don't translate that properly)Can we detect WSL and only wait prematurely when really needed?
You are right. I'm not sure what is going on in WSL. Everything is fine in MinGW. I will test Linux later.
detect WSL and workaround Popen issue
Please try the patch above, according to Provide a way to positively detect WSL from an app compiled on Linux, this is the correct way - untested.
I don't really understand this bit from your patch:
# without this the return code is always None in WSL (and becomes zero after communicate())
The returncode always starts as None, it is set after one either calls wait
or communicate
, and that's true on all platforms AFAIK. So what is different with WSL?
This patch works. I apologize for not having had time to follow up more rapidly.
After your comment about SIGPIPE I doubted my initial observations.
I can confirm that it is broken in OpenSuSE 42.3 WSL and Ubuntu 18.04 WSL. I can also confirm that this patch fixes everything.
Thanks.
I can also confirm that this patch fixes everything.
Applied in r21175, r21176 for v2.4.x
At some point in the future they are likely to fix the WSL bug, and then we'll have to remove the workaround.
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/2059