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

client.stdin.close() does not actually close stdin #322

Closed
brandon-rhodes opened this issue May 2, 2014 · 6 comments
Closed

client.stdin.close() does not actually close stdin #322

brandon-rhodes opened this issue May 2, 2014 · 6 comments

Comments

@brandon-rhodes
Copy link

@brandon-rhodes brandon-rhodes commented May 2, 2014

When scripting interaction with a simple subprocess, closing stdin is often the only recognized way to indicate that the input is complete and may be acted upon by the subprocess.

from subprocess import Popen, PIPE
p = Popen(['cat'], stdin=PIPE, stdout=PIPE)
p.stdin.write('Hello!\n')
p.stdin.close()
print p.stdout.read()

Unfortunately, paramiko makes the above interaction nearly impossible with a remote cat because its channel's stdin.close() fails to actually cause a close of the file descriptor on the remote end.

This script hangs forever:

import paramiko
client = paramiko.SSHClient()
client.connect('localhost', username='brandon')
stdin, stdout, stderr = client.exec_command('cat')
stdin.write('Hello, world.\n')
stdin.close()  # fails to: channel.shutdown_write()
print stdout.read()
client.close()

Fortunately, Stack Overflow indicated that this issue can be worked around by (as shown in the comment above) reaching into stdin.channel and performing the close manually. But it would be an improvement to paramiko for its own close() to natively implement the same behavior as local file descriptors show in subprocess. Thanks!

@banjaxedben
Copy link

@banjaxedben banjaxedben commented Oct 1, 2015

Is this bug related to #451 ?

@joernheissler
Copy link

@joernheissler joernheissler commented Jan 22, 2017

In #451 they call channel.shutdown_write, so yes, it's kind of related.
But still a different issue.

I'm new to paramiko and I stumbled across this issue very fast. Tried stdin.close() and it didn't work, so I found this bug report.

I also think that BufferedFile.close() should close/shutdown its underlying channel.

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 14, 2019

Taking a quick peek at this since it was linked elsewhere, though this isn't actually the problem so much as it was a guide to a solution over there (fabric/fabric#1985 / pyinvoke/invoke#552).

Took me a minute to find this but part of the issue is that Client itself isn't responsible for calling shutdown_write, it's the pseudofile returned by exec_command - which is paramiko.channel.ChannelFile. That currently delegates .close to its parent class paramiko.file.BufferedFile, which just flushes itself and does not understand channels at all (obviously - as that is what ChannelFile adds).

Should just require extending close and calling shutdown_write...?

@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 14, 2019

Lord above, Channel and ChannelFile have no tests of their own. 😩 I'm going to refer to this sort of thing as an inheritance tax from now on.

bitprophet added a commit that referenced this issue Jun 15, 2019
@bitprophet
Copy link
Member

@bitprophet bitprophet commented Jun 15, 2019

This is fixed now; better late than never.

For my downstreams, Fabric 1 users may benefit some from this; Fabric 2 itself doesn't use Client.exec_command or ChannelFile (it performs its own direct recv, recv_stderr etc on the Channel object in play; and thus (in abovelinked ticket) also does its own Channel.shutdown_write.

@bitprophet bitprophet closed this Jun 15, 2019
ploxiln added a commit to ploxiln/paramiko-ng that referenced this issue Jun 15, 2019
implement paramiko#322

SSHClient.exec_command() previously returned a naive
ChannelFile object for its stdin value; such objects
don't know to properly shut down the remote end's stdin when they
`.close()` - this leads to issues when running remote commands that read
from stdin.

A new subclass, ChannelStdinFile, has been created which
closes remote stdin when it itself is closed.
SSHClient.exec_command() has been updated to use that class
for its stdin return value.

Thanks to Brandon Rhodes for the report & steps to reproduce.
ploxiln added a commit to ploxiln/paramiko-ng that referenced this issue Jun 15, 2019
implement paramiko#322

SSHClient.exec_command() previously returned a naive
ChannelFile object for its stdin value; such objects
don't know to properly shut down the remote end's stdin when they
`.close()` - this leads to issues when running remote commands that read
from stdin.

A new subclass, ChannelStdinFile, has been created which
closes remote stdin when it itself is closed.
SSHClient.exec_command() has been updated to use that class
for its stdin return value.

Thanks to Brandon Rhodes for the report & steps to reproduce.
@brandon-rhodes
Copy link
Author

@brandon-rhodes brandon-rhodes commented Jun 17, 2019

I rarely see 5-year old bugs suddenly resolved, so thanks for digging and getting this settled — though I've moved on to other projects (I'm at this very moment thinking: what project was I using paramiko in 5 years ago?), this is a nice step forward for the project!

ploxiln added a commit to ploxiln/paramiko-ng that referenced this issue Jul 1, 2019
implement paramiko#322

SSHClient.exec_command() previously returned a naive
ChannelFile object for its stdin value; such objects
don't know to properly shut down the remote end's stdin when they
`.close()` - this leads to issues when running remote commands that read
from stdin.

A new subclass, ChannelStdinFile, has been created which
closes remote stdin when it itself is closed.
SSHClient.exec_command() has been updated to use that class
for its stdin return value.

Thanks to Brandon Rhodes for the report & steps to reproduce.
ploxiln added a commit to ploxiln/paramiko-ng that referenced this issue Jan 20, 2020
implement paramiko#322

SSHClient.exec_command() previously returned a naive
ChannelFile object for its stdin value; such objects
don't know to properly shut down the remote end's stdin when they
`.close()` - this leads to issues when running remote commands that read
from stdin.

A new subclass, ChannelStdinFile, has been created which
closes remote stdin when it itself is closed.
SSHClient.exec_command() has been updated to use that class
for its stdin return value.

Thanks to Brandon Rhodes for the report & steps to reproduce.
ploxiln added a commit to ploxiln/paramiko-ng that referenced this issue Jan 20, 2020
implement paramiko#322

SSHClient.exec_command() previously returned a naive
ChannelFile object for its stdin value; such objects
don't know to properly shut down the remote end's stdin when they
`.close()` - this leads to issues when running remote commands that read
from stdin.

A new subclass, ChannelStdinFile, has been created which
closes remote stdin when it itself is closed.
SSHClient.exec_command() has been updated to use that class
for its stdin return value.

Thanks to Brandon Rhodes for the report & steps to reproduce.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants