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

Andrew Bennetts (spiv) 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>.

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've rewritten your test so that it doesn't rely on timeouts or time.sleep, and so that it is an implementation test that runs against all of SmartTCPClientMedium, SmartSimplePipesClientMedium, and SmartSSHClientMedium. I do have to do a bit of ugly monkey-patching and fiddly thread interactions instead, but I think the resulting test will pass and fail with much greater reliability than one that can be affected by timing.

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.

I still need to do a bit more refactoring in my branch to remove duplication with existing test code, but this has finally pushed me into making per-medium implementation tests, so thank you! I'll probably put this new test in a new per_smart_medium test module.

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.

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?

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

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've rewritten your test so that it doesn't rely on timeouts or time.sleep, and so that it is an implementation test that runs against all of SmartTCPClientMedium, SmartSimplePipesClientMedium, and SmartSSHClientMedium.  I do have to do a bit of ugly monkey-patching and fiddly thread interactions instead, but I think the resulting test will pass and fail with much greater reliability than one that can be affected by timing.

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.

I still need to do a bit more refactoring in my branch to remove duplication with existing test code, but this has finally pushed me into making per-medium implementation tests, so thank you!  I'll probably put this new test in a new per_smart_medium test module.

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.

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?

review: Needs Fixing

« Back to merge proposal