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

Patch to enable SSH-based remote command executor for windows clients #73

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

mih
Copy link
Member

@mih mih commented Sep 19, 2023

This patch aims to fix a hanging Python sessions after the execution
of an SSH remote command call with no particular stdin input.

Interpretation from #68

Without overriding stdin, the subprocess, i.e. ssh, and python share
the same file pointer. It seems that stdin is configured in a way that
unexpected by the interpreter and messes with python's way to read from
sys.stdin.

This patch passes an explicit b'' as stdin to the SSH client
execution process to effectively achieve a separate fiel descriptor for
that client process.

This patch should not interfere with the implementation of the sshrun
command in datalad-core. It uses a dedicated not-None value for any
execution. However, the compatibility and interference of this patch
should be subject to a thorough investigation and widespread testing
before this changeset is proposed for datalad-core.

Towards fixing #68

This patch aims to fix a hanging Python sessions after the execution
of an SSH remote command call with no particular stdin input.

Interpretation from #68

    Without overriding stdin, the subprocess, i.e. ssh, and python share
    the same file pointer. It seems that stdin is configured in a way that
    unexpected by the interpreter and messes with python's way to read from
    sys.stdin.

This patch passes an explicit `b''` as `stdin` to the SSH client
execution process to effectively achieve a separate fiel descriptor for
that client process.

This patch should not interfere with the implementation of the `sshrun`
command in datalad-core. It uses a dedicated not-None value for any
execution. However, the compatibility and interference of this patch
should be subject to a thorough investigation and widespread testing
before this changeset is proposed for datalad-core.

Closes #68
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Patch coverage: 95.23% and project coverage change: -0.36% ⚠️

Comparison is base (887bc23) 97.88% compared to head (7ed2c2e) 97.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
- Coverage   97.88%   97.53%   -0.36%     
==========================================
  Files          10       13       +3     
  Lines         142      162      +20     
==========================================
+ Hits          139      158      +19     
- Misses          3        4       +1     
Files Changed Coverage Δ
datalad_ria/patches/ssh_exec.py 93.75% <93.75%> (ø)
datalad_ria/__init__.py 100.00% <100.00%> (ø)
datalad_ria/patches/__init__.py 100.00% <100.00%> (ø)
datalad_ria/patches/enabled.py 100.00% <100.00%> (ø)
datalad_ria/tests/test_ssh_connection.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mih mih merged commit 3f03f22 into main Sep 19, 2023
3 checks passed
@mih mih deleted the ssh-win branch September 19, 2023 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants