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

Already on GitHub? Sign in to your account

Handle/fix handling of EINTR errors in a few places. #40

Merged
merged 1 commit into from Sep 10, 2012

Conversation

Projects
None yet
2 participants
Contributor

DouglasTurk commented Sep 3, 2012

Hi,
I've encountered a few places where EINTR isn't getting handled, or isn't getting handled correctly (in packet.py), so I thought I'd submit a patch. Please let me know if there's any issues.
Thanks,
Doug Turk

Owner

bitprophet commented Sep 8, 2012

Thanks, this looks good overall, and grateful you included a test too :)

I have a basic understanding of what EINTR is but don't recall running into it in an SSH context, personally. Can you provide some examples of real-world situations where you've run into this & how ssh/paramiko behaved prior to your patch? (Specific shell output would be great, but even a general description is fine.)

Contributor

DouglasTurk commented Sep 10, 2012

EINTR is returned by various system calls when a thread performs a blocking operation (like connecting a socket, or reading/writing to/from a socket), and the kernel delivers a signal to the blocked thread, mid-way through the wait. The frequency of signals like this really depends on the type of program; programs that do things like run child processes and then make an ssh connection might have to deal with this more than most programs. The symptoms are occasional failures on blocking operations, where a socket.error exception is raised with an error message of "interrupted system call". I've seen this on some of the socket.connect calls. Also, there was already code in packet.py to handle EINTR, but it was a bit incorrect - if EINTR was raised the first time through the retry loop, the code would not assign a value to "n", and would raise an "unbound local" exception with a stack trace pointing to that function.

Owner

bitprophet commented Sep 10, 2012

Thanks, that makes sense. Merging :)

@bitprophet bitprophet added a commit that referenced this pull request Sep 10, 2012

@bitprophet bitprophet Merge pull request #40 from DouglasTurk/eintr_fixes
Handle/fix handling of EINTR errors in a few places.
6560da8

@bitprophet bitprophet merged commit 6560da8 into bitprophet:master Sep 10, 2012

@bitprophet bitprophet added a commit that referenced this pull request Sep 10, 2012

@bitprophet bitprophet Changelog re #40 c27915d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment