Merge lp://staging/~blake-rouse/maas/fix-1602721 into lp://staging/~maas-committers/maas/trunk

It's too bad we have to hack around piston in this way. I think it would be acceptable to land this, but I'd like to see what others think, too. (Is this the best workaround we can think of? Should we consider a monkey patch, followed by submitting a patch upstream?)

The comment in the test for __eq__ is a code smell; I think the test either needs more testing to cover all the possible combinations, or be changed to be what we expect Piston to do. See comment below.

I think this is much safer than monkey patching piston. Monkey patching twisted will affect all of MAAS not just this one endpoint that has this problem. I prefer to keeping this issue isolated and work around the problem, instead of monkey patching piston and affecting all of MAAS.

I fixed the compare and still assertNotEqual is doing something wierd.

I was thinking about this bug more, and I think I still have more questions than answers. I think this may point to a larger issue in Piston.

I assume that Piston uses something like id() to determine if an object is a circular reference. (In other words, the `is` keyword.)

It seems that the Python interpreter automatically caches short strings for re-use, using `sys.intern()`. Watch:

>>> id("")
4337834672
>>> id("")
4337834672

>>> id("abcd")
4340070528
>>> id("abcd")
4340070528

>>> id("abcd" * 2)
4340076784
>>> id("abcd" * 2)
4340076784

>>> id("abcd" * 4)
4340075952
>>> id("abcd" * 4)
4340075952

>>> id("abcd" * 8)
4340006032
>>> id("abcd" * 8)
4340006032

>>> id("abcd" * 16)
4340092976
>>> id("abcd" * 16)
4340092976

>>> id("abcd" * 32)
4338814056
>>> id("abcd" * 32)
4338814056

>>> id("abcd" * 64)
4338324752
>>> id("abcd" * 64)
4338324752

>>> id("abcd" * 128)
140650001490496
>>> id("abcd" * 128)
140649999734256

(So in this test it starts returning different objects only when the string length reaches over ~512 bytes.)

If you try the same with an empty string that you read from /dev/null, here's what you get:

>>> f = open('/dev/null', 'r')
>>> null = f.read()
>>> id(null)
4337834672
>>> null
''
>>> id('')
4337834672
>>> '' is null
True

The object has the same ID -- so somewhere in the code, they returned the cached string constant that stands for "empty string". Therefore the Python `is` operator returns True, which (for circular reference checking) is a bit unexpected because you would think they would be different objects.

Here's the fun part. Try the same experiment, but this time read *bytes* from /dev/null rather than a string:

>>> f = open('/dev/null', 'rb')
>>> null = f.read()
>>> null
b''
>>> id(null)
140649999594096
>>> id(b'')
4337964736
>>> b'' is null
False

This time, the b'' is *different* -- it points to an object with a different underlying memory address.

If you really want a unique object, (not interned) I think there is a workaround cleaner than reading a byte string from /dev/null: use `io.BytesIO`.

>>> a = bytes(BytesIO())
>>> b = bytes(BytesIO())
>>> c = bytes(BytesIO())
>>> id(b'')
4337964736
>>> id(a)
4339359696
>>> id(b)
4339359056
>>> id(c)
4339359536

You get a different object back each time.

So maybe the real fix is to not use the constant b'', and instead return a unique object for Piston to use, so that when it does its circular reference checks it doesn't get tripped up.

My fear is that this problem isn't isolated to b'', and could happen anywhere we have zero-length or short strings (especially strings that have been hard-coded somewhere, such as the empty string).

I dug more into the issue and figured out that it was possible that it would affect other areas of MAAS and it all had to do with how __emittable__ was defined on the Bin class. See the code and the comments to understand better why this is needed.

Monkey patching piston3 is not an option as the the whole emitter module in piston3 is one huge function will functions defined in that function. We have thought about just pulling piston3 into MAAS, but now is not a time to do this since this needs to go back into 2.0.

I toke Gavin's approach. Please take another look.

LGTM. Thanks!