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

Proposed by Blake Rouse
Status: Merged
Approved by: Blake Rouse
Approved revision: 5199
Merged at revision: 5206
Proposed branch: lp:~blake-rouse/maas/fix-1602721
Merge into: lp:maas/trunk
Diff against target: 68 lines (+33/-3)
3 files modified
src/maasserver/api/tests/test_commissioning.py (+30/-0)
src/metadataserver/fields.py (+1/-1)
src/metadataserver/tests/test_fields.py (+2/-2)
To merge this branch: bzr merge lp:~blake-rouse/maas/fix-1602721
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Approve
Gavin Panella (community) Approve
Blake Rouse (community) Approve
Review via email: mp+300803@code.launchpad.net

Commit message

Prevent Bin class from causing Piston3 from having a circular dependency issue. An object that defines __emittable__ can not return an equal object as a result or a circular dependency will occur.

To post a comment you must log in.
Revision history for this message
Mike Pontillo (mpontillo) wrote :

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.

review: Needs Information
Revision history for this message
Blake Rouse (blake-rouse) wrote :

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.

lp:~blake-rouse/maas/fix-1602721 updated
5197. By Blake Rouse on 2016-07-21

Compare NeverEqualBin to NeverEqualBin.

Revision history for this message
Blake Rouse (blake-rouse) wrote :

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

Revision history for this message
Mike Pontillo (mpontillo) wrote :

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).

lp:~blake-rouse/maas/fix-1602721 updated
5198. By Blake Rouse on 2016-07-22

Move the logic to the Bin class as it affects the whole class.

Revision history for this message
Blake Rouse (blake-rouse) wrote :

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.

Revision history for this message
Gavin Panella (allenap) :
Revision history for this message
Gavin Panella (allenap) :
lp:~blake-rouse/maas/fix-1602721 updated
5199. By Blake Rouse on 2016-07-26

Fix code review.

Revision history for this message
Blake Rouse (blake-rouse) wrote :

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

review: Approve
Revision history for this message
Gavin Panella (allenap) :
review: Approve
Revision history for this message
Mike Pontillo (mpontillo) wrote :

LGTM. Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maasserver/api/tests/test_commissioning.py'
2--- src/maasserver/api/tests/test_commissioning.py 2016-05-24 14:43:27 +0000
3+++ src/maasserver/api/tests/test_commissioning.py 2016-07-26 10:03:51 +0000
4@@ -173,6 +173,36 @@
5 ]
6 )
7
8+ def test_list_returns_with_multiple_empty_data(self):
9+ commissioning_results = [
10+ factory.make_NodeResult_for_commissioning(data=b"")
11+ for counter in range(3)]
12+ url = reverse('node_results_handler')
13+ response = self.client.get(url)
14+ self.assertEqual(
15+ http.client.OK, response.status_code, response.content)
16+ parsed_results = json_load_bytes(response.content)
17+ self.assertItemsEqual(
18+ [
19+ (
20+ commissioning_result.name,
21+ commissioning_result.script_result,
22+ b64encode(commissioning_result.data).decode("utf-8"),
23+ commissioning_result.node.system_id,
24+ )
25+ for commissioning_result in commissioning_results
26+ ],
27+ [
28+ (
29+ result.get('name'),
30+ result.get('script_result'),
31+ result.get('data'),
32+ result.get('node').get('system_id'),
33+ )
34+ for result in parsed_results
35+ ]
36+ )
37+
38 def test_list_can_be_filtered_by_node(self):
39 commissioning_results = [
40 factory.make_NodeResult_for_commissioning()
41
42=== modified file 'src/metadataserver/fields.py'
43--- src/metadataserver/fields.py 2015-12-02 20:43:30 +0000
44+++ src/metadataserver/fields.py 2016-07-26 10:03:51 +0000
45@@ -57,7 +57,7 @@
46
47 Exists as a hook for Piston's JSON encoder.
48 """
49- return b64encode(self)
50+ return b64encode(self).decode('ascii')
51
52
53 class BinaryField(Field, metaclass=SubfieldBase):
54
55=== modified file 'src/metadataserver/tests/test_fields.py'
56--- src/metadataserver/tests/test_fields.py 2015-12-01 18:12:59 +0000
57+++ src/metadataserver/tests/test_fields.py 2016-07-26 10:03:51 +0000
58@@ -35,10 +35,10 @@
59 # Piston hooks onto an __emittable__() method, if present.
60 # Bin() returns a base-64 encoded string so that it can be
61 # transmitted in JSON.
62- self.assertEqual(b"", Bin(b"").__emittable__())
63+ self.assertEqual("", Bin(b"").__emittable__())
64 example_bytes = factory.make_bytes()
65 self.assertEqual(
66- b64encode(example_bytes),
67+ b64encode(example_bytes).decode('ascii'),
68 Bin(example_bytes).__emittable__())
69
70