xpra icon
Bug tracker and wiki

Opened 3 weeks ago

Closed 2 weeks ago

#2059 closed defect (fixed)

host key dialog does not read "yes" or "no" correctly

Reported by: Nathan Hallquist Owned by: Nathan Hallquist
Priority: major Milestone: 2.5
Component: client Version: 2.4.x
Keywords: Cc:

Description

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.

Attachments (2)

hd.txt (567 bytes) - added by Nathan Hallquist 3 weeks ago.
wsl.patch (1.8 KB) - added by Antoine Martin 2 weeks ago.
detect WSL and workaround Popen issue

Download all attachments as: .zip

Change History (7)

Changed 3 weeks ago by Nathan Hallquist

Attachment: hd.txt added

comment:1 Changed 3 weeks ago by Antoine Martin

Owner: changed from Antoine Martin to Nathan Hallquist

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 SIGPIPEs.

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?

comment:2 in reply to:  1 Changed 3 weeks ago by Nathan Hallquist

Replying to Antoine Martin:

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 SIGPIPEs.

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?

You are right. I'm not sure what is going on in WSL. Everything is fine in MinGW. I will test Linux later.

Changed 2 weeks ago by Antoine Martin

Attachment: wsl.patch added

detect WSL and workaround Popen issue

comment:3 Changed 2 weeks ago by Antoine Martin

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?

Last edited 2 weeks ago by Antoine Martin (previous) (diff)

comment:4 Changed 2 weeks ago by Nathan Hallquist

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.

comment:5 Changed 2 weeks ago by Antoine Martin

Resolution: fixed
Status: newclosed

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.

Note: See TracTickets for help on using tickets.