Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
totaam opened this issue Nov 22, 2018 · 10 comments
Closed

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

totaam opened this issue Nov 22, 2018 · 10 comments
Labels

Comments

@totaam
Copy link
Collaborator

totaam commented Nov 22, 2018

Issue migrated from trac ticket # 2059

component: client | priority: major | resolution: fixed

2018-11-22 21:36:06: nathan_lstc created the issue


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.

@totaam
Copy link
Collaborator Author

totaam commented Nov 22, 2018

2018-11-22 21:37:11: nathan_lstc uploaded file hd.txt (0.6 KiB)

@totaam
Copy link
Collaborator Author

totaam commented Nov 23, 2018

2018-11-23 05:32:40: antoine changed owner from antoine to nathan_lstc

@totaam
Copy link
Collaborator Author

totaam commented Nov 23, 2018

2018-11-23 05:32:40: antoine commented


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?

@totaam
Copy link
Collaborator Author

totaam commented Nov 24, 2018

2018-11-24 00:14:11: nathan_lstc commented


Replying to [comment:1 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.

@totaam
Copy link
Collaborator Author

totaam commented Nov 30, 2018

2018-11-30 10:36:42: antoine uploaded file wsl.patch (1.8 KiB)

detect WSL and workaround Popen issue

@totaam
Copy link
Collaborator Author

totaam commented Nov 30, 2018

2018-11-30 10:37:57: antoine commented


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?

@totaam
Copy link
Collaborator Author

totaam commented Nov 30, 2018

2018-11-30 20:58:24: nathan_lstc commented


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.

@totaam
Copy link
Collaborator Author

totaam commented Dec 1, 2018

2018-12-01 04:05:26: antoine changed status from new to closed

@totaam
Copy link
Collaborator Author

totaam commented Dec 1, 2018

2018-12-01 04:05:26: antoine set resolution to fixed

@totaam
Copy link
Collaborator Author

totaam commented Dec 1, 2018

2018-12-01 04:05:26: antoine commented


I can also confirm that this patch fixes everything.
Applied in r21175, 21176 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.

@totaam totaam closed this as completed Dec 1, 2018
@totaam totaam added the v2.4.x label Jan 22, 2021
@totaam totaam mentioned this issue Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant