First crude smoke test for SSHRemoteIO #69
Conversation
mih
force-pushed
the
tst-remoteio
branch
2 times, most recently
from
483ce89
to
88401a6
Compare
|
Tests cannot run ATM, because of some certificate expiration issue on the windows working images of appveyor. Time will fix it, I guess. |
|
I have just a preliminary observation: The test in this PR here doesn't run through on my Windows machine. The patch is applied ( In a python session, things look as follows: doesn't do anything (seemlingly) BUT I can CTRL-C it in the Python process and reuse the python session afterwards. So its not the same hanging as before. |
If I get into this while loop in a debugger, stdout only contains empty lines, so it seems trapped in there. |
Thanks for tracking that down. [Edit] We should definitely check for that condition and probably raise an error, that shows the content of |
I'm getting a few conflicting findings, maybe you can make sense of them: I interpreted the "is_open() -> False" as weird and an indication that there could be a connection issue.
the Edit: If I hardcode the port (sshuser@localhost:2222), I still end up in a seemingly endless loop. |
That is definitely wrong. There is some old code, i.e.
Can you try with If that works, we just have to fix the (IMHO needlessly complicated) SSHRI code (or, if that would break datalad, bundle an updated version with a patched |
We have to check whether the shell process has returned with a non-zero value in the loop. The while True:
status = self.shell.poll()
if status not in (0, None):
raise Exception(f'ssh exited with status {status}') # TODO use proper exception class
line = self.shell.stdout.readline()
if line == '':
raise Exception(f'ssh closed stdout unexpectedly')
if line == b"RIA-REMOTE-LOGIN-END\n":
breakThat should leave the loop if any errors occur. |
|
I am preparing a commit, that includes the necessary patches. Let's see how that works. |
|
re general note re "connection open" etc.: Please keep in mind that on Windows SSH does not do connection multiplexing. This means that there is never an open connection, and all this code was likely written with the assumption that such oddity cannot exist. I suspect that these flags have little meaning on windows and they are coded up to make-pretend and satisfy other code's assumptions on how the world should behave (but isn't). |
|
I have extracted everything from this PR that is not narrowly focused on
They have been merged into |
christian-monch
force-pushed
the
tst-remoteio
branch
from
f85c422
to
32936d7
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 97.53% 96.93% -0.60%
==========================================
Files 13 15 +2
Lines 162 196 +34
==========================================
+ Hits 158 190 +32
- Misses 4 6 +2
☔ View full report in Codecov by Sentry. |
|
This is looking good now! I left a comment re documenting the path forward. But otherwise we should get this merged! It would be good to rebase this branch, and squash the history to the relevant bits. Likely just two commits:
WDYT? |
See if the whole shebang of identityfile, custom username, with custom port still yields a working IO abstraction.
christian-monch
force-pushed
the
tst-remoteio
branch
from
d539998
to
73f0225
Compare
This commit adds a patch that replaces SSHRemoteIO.__init__() with an udated version. The updated version uses proper arguments to open a the shell-ssh, if the ssh-connection is an instance of NoMultiplexSSHConnection.
christian-monch
force-pushed
the
tst-remoteio
branch
from
73f0225
to
f079c09
Compare
Document unavoidable deviations.
See if the whole shebang of identityfile, custom username, with custom port still yields a working IO abstraction.
This changeset includes a patch to datalad-core:
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
This patch passes an explicit
b''asstdinto the SSH clientexecution process to effectively achieve a separate fiel descriptor for
that client process.
This patch should not interfere with the implementation of the
sshruncommand 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