Proxycommand fix #681

Closed
wants to merge 3 commits into
from

Projects

None yet

6 participants

@radssh
Contributor
radssh commented Feb 7, 2016

Simple fix of using unbuffered subprocess.PIPE uncovered a trickier bug
where the lack of timeout on initial readline call would pull data from
the first real packet, throwing off subsequent reads.

Doesn’t seem necessary to preserve a read buffer across recv() calls,
especially if it isn’t implemented correctly.

radssh added some commits Aug 21, 2015
@radssh radssh Merge remote-tracking branch 'paramiko/master' c686e41
@radssh radssh Merge remote-tracking branch 'paramiko/master' 8f284f5
@radssh radssh ProxyCommand fixes for Python3
Simple fix of using unbuffered subprocess.PIPE uncovered a trickier bug
where the lack of timeout on initial readline call would pull data from
the first real packet, throwing off subsequent reads.

Doesn’t seem necessary to preserve a read buffer across recv() calls,
especially if it isn’t implemented correctly.
c638ecf
@rsuciu-gsg

Are there any updates on this?
I have also the same issue when running Python 3 (although it works fine in 2.7).

Thanks.

@ledil
ledil commented Jul 18, 2016

any news ? Ive got the same problem ...

@nvgoldin

same here.
any news?

@nvgoldin

Confirming this patch fixed the issue, however I have another issue now, not sure if its related:

  File ".../python3.5/site-packages/paramiko/transport.py", line 1550, in stop_thread
    and not self.sock._closed and not self.packetizer.closed
AttributeError: 'ProxyCommand' object has no attribute '_closed'

I think this was added recently to transport.py in: 0ebd247

            while (
                self.is_alive() and self is not threading.current_thread()
                and not self.sock._closed and not self.packetizer.closed
            ):
                self.join(0.1)
@bitprophet
Member
bitprophet commented Jul 20, 2016 edited

@nvgoldin that secondary issue is #774, FYI.

I haven't had time to test this fix myself; if @ledil and @rsuciu-gsg can confirm @nvgoldin's assert that it fixes the issue for them, I may consider merging w/o self-testing. Thanks!

@catmeme
catmeme commented Jul 20, 2016

Verifying this PR resolved this issue for me as well. Thanks @radssh

@rsuciu-gsg

Also works for me.

Thanks,
Ramona

On Wed, Jul 20, 2016 at 10:01 PM, catmeme notifications@github.com wrote:

Verifying this PR resolved this issue for me as well. Thanks @radssh
https://github.com/radssh


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#681 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANb3XK5DaDNDaWdl-zHkhckRSQjeVYaTks5qXnB8gaJpZM4HVNHN
.

@bitprophet bitprophet added this to the 1.16.3 et al milestone Jul 21, 2016
@bitprophet
Member

Thanks all. Merging on faith. Read the fix commit & commit message and squinted at the code real good...pretty sure I see why it's a good fix. Computers: difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment