Bazaar

Code review comment for lp:~gz/bzr/test_ssh_client_medium_eintr__read_bytes

test_ssh_client_medium_eintr__read_bytes
Merge into bzr.dev

Revision history for this message

Martin Packman (gz) wrote on 2010-02-18:

> First, I've got a branch that builds on this patch at
> <lp:~spiv/bzr/test_ssh_client_medium_eintr__read_bytes>.

Great, thanks for looking at this, the test needed to be less specific and monolithic if it was going to stick around.

> Ok, I see the issue. It appears it's only a problem with the SSH medium, not
> the simple pipes (--inet) or TCP media. I haven't finished digging, but it
> appears that the root cause is that socket_object.makefile() is returning a
> file object that is buggy when EINTR occurs? If so, I think we can workaround
> that without losing the ability to handle EINTR.

I urge you to read the mailing list thread if you've not already, it included a complete list of which interfaces are affected and how in the first post. The only tricky bit is indeed SmartSSHClientMedium but the best fix is already upstream.

> I also changed the test to check for the existence of the EINTR symbol rather
> than just checking the platform name. So now test should run on POSIX-like
> platforms that don't say os.name == 'posix', yet might be vulnerable to this
> bug.

That is incorrect. EINTR *exists* on other platforms but this PC loser-ing issue is specific to unix.

> Finally, your test is a bit overly precise about the interface. It's ok for
> _read_bytes to return too few (but not 0 unless the connection has ended), or
> even too many bytes. Of course, of the bytes it returns, it has to return
> them in the right order, so I've fixed the test to assert just that.

It's demonstrating a specific problem with one implementation, poking it to work with others too is fine.

> I'm not totally satisfied with the shape of the new test, but I think it's
> getting closer, and it does clearly pinpoint the bug. Thanks for your
> persistence on this issue!

Your changes keeps around an interface I'm trying to junk and is a little harder to reason about, but losing the sleeps is good. Please CC me on the review when you post it, as I've some specific comments.

> We may to backport the eventual fix to 2.0, if it isn't too hairy. Thinking
> of which, is there a bug number for this?

Nope, given that there are about five different bugs with the same underlying cause, it seemed easier to take it to the mailing list so all the discussion stayed in one place.

> First, I've got a branch that builds on this patch at
> <lp:~spiv/bzr/test_ssh_client_medium_eintr__read_bytes>.

Great, thanks for looking at this, the test needed to be less specific and monolithic if it was going to stick around.

> Ok, I see the issue.  It appears it's only a problem with the SSH medium, not
> the simple pipes (--inet) or TCP media.  I haven't finished digging, but it
> appears that the root cause is that socket_object.makefile() is returning a
> file object that is buggy when EINTR occurs?  If so, I think we can workaround
> that without losing the ability to handle EINTR.

> I also changed the test to check for the existence of the EINTR symbol rather
> than just checking the platform name.  So now test should run on POSIX-like
> platforms that don't say os.name == 'posix', yet might be vulnerable to this
> bug.

That is incorrect. EINTR *exists* on other platforms but this PC loser-ing issue is specific to unix.

> Finally, your test is a bit overly precise about the interface.  It's ok for
> _read_bytes to return too few (but not 0 unless the connection has ended), or
> even too many bytes.  Of course, of the bytes it returns, it has to return
> them in the right order, so I've fixed the test to assert just that.

It's demonstrating a specific problem with one implementation, poking it to work with others too is fine.

> I'm not totally satisfied with the shape of the new test, but I think it's
> getting closer, and it does clearly pinpoint the bug.  Thanks for your
> persistence on this issue!

> We may to backport the eventual fix to 2.0, if it isn't too hairy.  Thinking
> of which, is there a bug number for this?

Nope, given that there are about five different bugs with the same underlying cause, it seemed easier to take it to the mailing list so all the discussion stayed in one place.

« Back to merge proposal