[MERGE] Fix test suite regression on OSX in TestReadMergeableFromUrl.test_smart_server_connection_reset.

[Vincent Ladeuil]

>>>>> "Andrew" == Andrew Bennetts <andrew at canonical.com> writes:

    Andrew> Vincent Ladeuil wrote:
    >> revno 3703.2.1 'Allow ConnectionReset to propagate from
    >> read_mergeable_from_url' added a test that fails on OSX.
    >> 
    >> It fails sometimes only as is often the case when sockets are
    >> involved (python on OSX often produces different execution orders
    >> when several threads are involved) which made it a bit hard to
    >> reproduce and may explain why it doesn't seem to occur on linux.
    >> 
    >> The attached patch fixed the issue but I'd appreciate if Andrew
    >> can review it since it may not be the right approach (or more
    >> precisely I suspect there is a better one or more complete one
    >> but I can't put my finger on it).

    Andrew> bb:resubmit

    Andrew> Out of interest, what does the stacktrace for the failing test look
    Andrew> like?  I think I can guess, but it'd be good to avoid unnecessary
    Andrew> guesswork :)

Sure:

ERROR: test_smart_server_connection_reset (bzrlib.tests.test_bundle.TestReadMergeableFromUrl)

vvvv[log from bzrlib.tests.test_bundle.TestReadMergeableFromUrl.test_smart_server_connection_reset]
1.807  opening working tree '/tmp/testbzr-rk_X_U.tmp'

^^^^[log from bzrlib.tests.test_bundle.TestReadMergeableFromUrl.test_smart_server_connection_reset]
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/v/home/vila/src/bzr/trunk/bzrlib/tests/test_bundle.py", line 1602, in test_smart_server_connection_reset
  File "/v/home/vila/src/bzr/trunk/bzrlib/tests/__init__.py", line 966, in assertRaises
  File "/v/home/vila/src/bzr/trunk/bzrlib/bundle/__init__.py", line 53, in read_mergeable_from_url
  File "/v/home/vila/src/bzr/trunk/bzrlib/bundle/__init__.py", line 75, in read_mergeable_from_transport
  File "/v/home/vila/src/bzr/trunk/bzrlib/transport/__init__.py", line 1621, in do_catching_redirections
  File "/v/home/vila/src/bzr/trunk/bzrlib/bundle/__init__.py", line 63, in get_bundle
  File "/v/home/vila/src/bzr/trunk/bzrlib/transport/remote.py", line 202, in get
  File "/v/home/vila/src/bzr/trunk/bzrlib/transport/remote.py", line 207, in get_bytes
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/client.py", line 135, in call_expecting_body
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/client.py", line 83, in _call_and_read_response
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/message.py", line 258, in read_response_tuple
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/message.py", line 224, in _wait_for_response_args
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/message.py", line 237, in _read_more
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/medium.py", line 439, in read_bytes
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/medium.py", line 451, in _read_bytes
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/medium.py", line 143, in read_bytes
  File "/v/home/vila/src/bzr/trunk/bzrlib/smart/medium.py", line 787, in _read_bytes
error: (54, 'Connection reset by peer')

    Andrew> I don't think this is the right fix.  This seems to
    Andrew> be the wrong layer to be catching socket.errors.  I
    Andrew> think we really need to make the various medium
    Andrew> implementations present a consistent interface as far
    Andrew> as possible, or we'll inevitably have to put this
    Andrew> sort of band-aid in other places.  It's the medium
    Andrew> that should be translating the low-level details of
    Andrew> delivering and receiving bytes into a simple
    Andrew> read_bytes/accept_bytes interface.  socket.errors
    Andrew> should be an implementation detail of one particular
    Andrew> medium.

Bah, of course, silly me ;)

    Andrew> So I think the relevant SmartClientMedium (in this
    Andrew> case SmartTCPClientMedium I guess) should be catching
    Andrew> this error and translating it into something
    Andrew> consistent for the caller, which at the moment would
    Andrew> be to return ''.  So that would mean updating
    Andrew> SmartTCPClientMedium._read_bytes.  (Or
    Andrew> SmartClientHTTPMediumRequest._read_bytes?)

Done in SmartTCPClientMedium. Which leads me to the forgotten
question: in what circumstances is the smart involved in reading
a *bundle* ?

    Andrew> I'm starting to think that the right thing to do
    Andrew> would be to make the medium become responsible for
    Andrew> raising ConnectionReset, rather than the caller of
    Andrew> read_bytes.  This is probably a deeper change than
    Andrew> necessary for fixing this particular bug, so I
    Andrew> wouldn't demand it happen immediately.  But I think
    Andrew> we should be moving in this direction.

And how far should we go there ? 'ConnectionReset', as far as
hpss is concerned can be viewed as a transient error, should we
reconnect automatically (even harder and higher in the stack I
guess) ?

>>>>> "Andrew" == Andrew Bennetts <andrew at canonical.com> writes:

    Andrew> Vincent Ladeuil wrote:
    Andrew> [...]
    >> +            if len(e.args) and e.args[0] is errno.ECONNRESET:

    Andrew> In addition to my other comments, don't use “is” to
    Andrew> compare these integers, just use “==”.  It'll
    Andrew> probably work all the time with all current
    Andrew> implementations of CPython, but we shouldn't be
    Andrew> relying on that.

I've been bitten by that in another project (I used 'is' here by
copy/paste, I should have known better :) and I was wondering:
using 'is' seems to be an optimized path when both strings are
interned, but '==' should knows that and use it, so is it really
worth trying to use 'is' ? And if yes, in what contexts ?


Leaving the discussion opened, here is an updated patch so that
the OSX support doesn't bitrot.

    Vincent


-------------- next part --------------
A non-text attachment was scrubbed...
Name: osx_tests_fix-3752.patch
Type: text/x-patch
Size: 3783 bytes
Desc: BZR merge
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081001/3af545e7/attachment.bin