THRIFT-4621 Add THeader for Python #1583

spladug · 2018-08-22T18:50:24Z

Client: py

This implements the same subset of THeaderProtocol that the C++ implementation does. Specifically, it supports TBinaryProtocol and TCompactProtocol payloads and allows those same protocols to come in without headers framed/unframed for backwards compatibility. It also supports the gzip transform.

Both Python 2 and Python 3 pass the full test suite (feature and cross) with C++ thrown in the mix:

$ make cross
/usr/bin/python3 test/test.py --retry-count 5 --features .* --skip-known-failures --server cpp,c_glib,,,,,py,py3,,,,,,,,,,,
...
No unexpected failures.
0 failed of 40 tests in total.
/usr/bin/python3 test/test.py --retry-count 5 --skip-known-failures --server cpp,c_glib,,,,,py,py3,,,,,,,,,,, --client cpp,c_glib,,,,,py,py3,,,,,,,,,,, --regex ".*"
...
No unexpected failures.
0 failed of 712 tests in total.

The only newly added known failures for this are those using the HTTP transport.

--- a/test/known_failures_Linux.json
+++ b/test/known_failures_Linux.json
@@ -83,6 +83,8 @@
   "cpp-py3_compact-accelc_http-ip-ssl",
   "cpp-py3_compact_http-ip",
   "cpp-py3_compact_http-ip-ssl",
+  "cpp-py3_header_http-ip",
+  "cpp-py3_header_http-ip-ssl",
   "cpp-py3_json_http-ip",
   "cpp-py3_json_http-ip-ssl",
   "cpp-py3_multi-accel_http-ip",
@@ -101,6 +103,8 @@
   "cpp-py3_multic-multiac_http-ip-ssl",
   "cpp-py3_multic_http-ip",
   "cpp-py3_multic_http-ip-ssl",
+  "cpp-py3_multih-header_http-ip",
+  "cpp-py3_multih-header_http-ip-ssl",
   "cpp-py3_multij-json_http-ip",
   "cpp-py3_multij-json_http-ip-ssl",
   "cpp-py3_multij_http-ip",
@@ -113,6 +117,8 @@
   "cpp-py_compact-accelc_http-ip-ssl",
   "cpp-py_compact_http-ip",
   "cpp-py_compact_http-ip-ssl",
+  "cpp-py_header_http-ip",
+  "cpp-py_header_http-ip-ssl",
   "cpp-py_json_http-ip",
   "cpp-py_json_http-ip-ssl",
   "cpp-py_multi-accel_http-ip",
@@ -131,6 +137,8 @@
   "cpp-py_multic-multiac_http-ip-ssl",
   "cpp-py_multic_http-ip",
   "cpp-py_multic_http-ip-ssl",
+  "cpp-py_multih-header_http-ip",
+  "cpp-py_multih-header_http-ip-ssl",
   "cpp-py_multij-json_http-ip",
   "cpp-py_multij-json_http-ip-ssl",
   "cpp-py_multij_http-ip",
@@ -375,6 +383,8 @@
   "py-cpp_binary_http-ip-ssl",
   "py-cpp_compact_http-ip",
   "py-cpp_compact_http-ip-ssl",
+  "py-cpp_header_http-ip",
+  "py-cpp_header_http-ip-ssl",
   "py-cpp_json_http-ip",
   "py-cpp_json_http-ip-ssl",
   "py-d_accel-binary_http-ip",
@@ -396,6 +406,7 @@
   "py-hs_accelc-compact_http-ip",
   "py-hs_binary_http-ip",
   "py-hs_compact_http-ip",
+  "py-hs_header_http-ip",
   "py-hs_json_http-ip",
   "py-java_accel-binary_http-ip",
   "py-java_accel-binary_http-ip-ssl",
@@ -420,6 +430,8 @@
   "py3-cpp_binary_http-ip-ssl",
   "py3-cpp_compact_http-ip",
   "py3-cpp_compact_http-ip-ssl",
+  "py3-cpp_header_http-ip",
+  "py3-cpp_header_http-ip-ssl",
   "py3-cpp_json_http-ip",
   "py3-cpp_json_http-ip-ssl",
   "py3-d_accel-binary_http-ip",
@@ -441,6 +454,7 @@
   "py3-hs_accelc-compact_http-ip",
   "py3-hs_binary_http-ip",
   "py3-hs_compact_http-ip",
+  "py3-hs_header_http-ip",
   "py3-hs_json_http-ip",
   "py3-java_accel-binary_http-ip",
   "py3-java_accel-binary_http-ip-ssl",

nsuke

@spladug thanks, it looks pretty nice overall !

nsuke · 2018-08-23T03:26:57Z

lib/py/src/transport/THeaderTransport.py

+import struct
+import zlib
+
+from thrift.compat import BufferIO, binary_to_str, str_to_binary, byte_index


nits: unused binary_to_str and str_to_binary (breaking style check CI job)

https://travis-ci.org/apache/thrift/jobs/419362288#L9859

nsuke · 2018-08-23T03:58:45Z

lib/py/src/server/TServer.py

@@ -158,7 +176,15 @@ def serveClient(self, client):
        itrans = self.inputTransportFactory.getTransport(client)
        otrans = self.outputTransportFactory.getTransport(client)


no need to create otrans if using THeader ?

nsuke · 2018-08-23T04:01:42Z

lib/py/src/server/TServer.py

+
+        # for THeaderProtocol, we must use the same protocol instance for input
+        # and output so that the response is in the same dialect that the
+        # server detected the request was in.


It would be nicer to inform the users that we're ignoring outputProtocolFactory if they explicitly provided one (self.inputProtocolFactory != self.outputProtocolFactory would suffice to tell if they did ?).

(same things for other servers)

Cool, makes sense. I've added a check to the TServer constructor to yell at the user if they explicitly pass a mix of THeaderProtocolFactory and another factory. Is that what you had in mind?

nsuke · 2018-08-24T03:00:00Z

lib/py/src/transport/THeaderTransport.py

+        self._client_type = THeaderClientType.HEADERS
+
+        if not allowed_client_types:
+            allowed_client_types = [THeaderClientType.HEADERS]


maybe just make it default argument.

nsuke · 2018-08-24T11:01:25Z

lib/py/src/transport/THeaderTransport.py

+
+    def set_header(self, key, value):
+        assert isinstance(key, bytes), "header names must be bytes"
+        assert isinstance(value, bytes), "header values must be bytes"


Raising TypeError would be nicer, IMO.

Wouldn't it make more sense to accept str for py3 ?

I didn't see anything in the spec about the payloads needing to be text, so I didn't want to arbitrarily limit what the header values could be; being raw bytes allows e.g. a BinaryProtocol struct to be encoded in there if desired. In fact, at Reddit we actually do send an encoded struct in our headers right now.

nsuke · 2018-08-24T11:12:37Z

lib/py/src/transport/THeaderTransport.py

+    return trans.read(length)
+
+
+def writeString(trans, value):


I think those 3 functions need at least _ prefix.
We could even define them inside function scope where we use them.

Since they're not closing around anything in the scope they're used, I felt it was cleaner/clearer to just put 'em out here. That OK?

nsuke · 2018-08-24T11:18:01Z

lib/py/src/transport/THeaderTransport.py

+        self.flags = 0
+        self.sequence_id = 0
+        self._protocol_id = THeaderSubprotocolID.BINARY
+        self.max_frame_size = 0x3FFFFFFF


Need _ prefix for this one since loosing up the limit can violate the protocol.

nsuke · 2018-08-24T12:40:04Z

lib/py/src/transport/THeaderTransport.py

+
+    @property
+    def cstringio_buf(self):
+        if not self._has_ever_read:


Curious what happens if we don't have this.

I need to look into this more. It's in the FBThrift implementation and to be honest I don't have a firm enough grasp on how the accelerated protocols interact with this to understand what might go wrong without it.

OK yeah, as far as I can tell it's unnecessary since the C code will call refill (which will read a frame) if this buffer is empty. Cool. Removed.

nsuke · 2018-08-24T12:45:16Z

lib/py/src/transport/THeaderTransport.py

+            payload = transform_fn(payload)
+            if transform_id not in write_transforms:
+                write_transforms.append(transform_id)
+        self._write_transforms = write_transforms


Not sure if it's a good idea.
It gets in your way if, e.g., you want to save your request bandwidth with extra CPU time on client, and not want to spend CPU time on server and happy to fill more response bandwidth.

I'm inclined not to do this.

Cool, I'll drop it. I was just mimicking FBThrift here but agree with your reasoning.

nsuke · 2018-08-24T12:50:01Z

lib/py/src/protocol/THeaderProtocol.py

+# specific language governing permissions and limitations
+# under the License.
+#
+


It would be nice to put a brief comment about unsupported features (like HTTP, and maybe non-blocking server) to save future users' time, here or somewhere noticeable.

spladug

Thanks for the feedback (and the super helpful prior art and tests!)

I think I've addressed all the comments so far. New changes are in new commits so you can see the differences more easily. I'll squash down the new commits before merge.

spladug · 2018-08-24T16:54:56Z

lib/py/src/transport/THeaderTransport.py

+
+    def set_header(self, key, value):
+        assert isinstance(key, bytes), "header names must be bytes"
+        assert isinstance(value, bytes), "header values must be bytes"


I didn't see anything in the spec about the payloads needing to be text, so I didn't want to arbitrarily limit what the header values could be; being raw bytes allows e.g. a BinaryProtocol struct to be encoded in there if desired. In fact, at Reddit we actually do send an encoded struct in our headers right now.

spladug · 2018-08-24T16:55:29Z

lib/py/src/transport/THeaderTransport.py

+            payload = transform_fn(payload)
+            if transform_id not in write_transforms:
+                write_transforms.append(transform_id)
+        self._write_transforms = write_transforms


Cool, I'll drop it. I was just mimicking FBThrift here but agree with your reasoning.

spladug · 2018-08-24T16:56:23Z

lib/py/src/server/TServer.py

+
+        # for THeaderProtocol, we must use the same protocol instance for input
+        # and output so that the response is in the same dialect that the
+        # server detected the request was in.


Cool, makes sense. I've added a check to the TServer constructor to yell at the user if they explicitly pass a mix of THeaderProtocolFactory and another factory. Is that what you had in mind?

spladug · 2018-08-24T17:04:11Z

lib/py/src/transport/THeaderTransport.py

+    return trans.read(length)
+
+
+def writeString(trans, value):


Since they're not closing around anything in the scope they're used, I felt it was cleaner/clearer to just put 'em out here. That OK?

nsuke · 2018-08-26T02:38:58Z

lib/py/src/protocol/THeaderProtocol.py

+
+    See doc/specs/HeaderFormat.md for details of the wire format.
+
+    """


great, thanks

Client: py

spladug · 2018-08-27T16:02:47Z

Thanks! All squashed down :)

jeking3 · 2018-08-29T01:54:03Z

Looks good to me as well, but need tests to pass. I can re-kick the Travis ones but I don't know if I can do that for Appveyor because someone else owns the Appveyor account.

nsuke · 2018-09-01T01:23:30Z

Appveyor has been working (now it's a download failure) before squash and Travis passed, so it should be OK.

spladug force-pushed the THRIFT-4621-THeader-for-Python branch from 4a76744 to 20605ce Compare August 22, 2018 20:37

nsuke reviewed Aug 24, 2018

View reviewed changes

spladug commented Aug 24, 2018

View reviewed changes

spladug force-pushed the THRIFT-4621-THeader-for-Python branch from 2e53e4b to 88de7e5 Compare August 24, 2018 20:31

nsuke approved these changes Aug 27, 2018

View reviewed changes

lib/py/src/protocol/THeaderProtocol.py

See doc/specs/HeaderFormat.md for details of the wire format.

"""

Copy link

Member

nsuke Aug 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks

THRIFT-4621 Add THeader for Python

fa9006b

Client: py

spladug force-pushed the THRIFT-4621-THeader-for-Python branch from c6634ed to fa9006b Compare August 27, 2018 16:02

jeking3 approved these changes Aug 29, 2018

View reviewed changes

nsuke merged commit 66a44c5 into apache:master Sep 1, 2018

spladug mentioned this pull request Aug 19, 2019

THRIFT-4621 Add THeader for Python spladug/thrift#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THRIFT-4621 Add THeader for Python #1583

THRIFT-4621 Add THeader for Python #1583

spladug commented Aug 22, 2018 •

edited

nsuke left a comment

nsuke Aug 23, 2018

nsuke Aug 23, 2018

nsuke Aug 23, 2018

nsuke Aug 24, 2018

spladug Aug 24, 2018

nsuke Aug 24, 2018

nsuke Aug 24, 2018

nsuke Aug 24, 2018

spladug Aug 24, 2018

nsuke Aug 24, 2018

spladug Aug 24, 2018

nsuke Aug 24, 2018

nsuke Aug 24, 2018

spladug Aug 24, 2018

spladug Aug 24, 2018

nsuke Aug 24, 2018 •

edited

spladug Aug 24, 2018

nsuke Aug 24, 2018

spladug left a comment

spladug Aug 24, 2018

spladug Aug 24, 2018

spladug Aug 24, 2018

spladug Aug 24, 2018

nsuke Aug 26, 2018

spladug commented Aug 27, 2018

jeking3 commented Aug 29, 2018

nsuke commented Sep 1, 2018

		@@ -158,7 +176,15 @@ def serveClient(self, client):
		itrans = self.inputTransportFactory.getTransport(client)
		otrans = self.outputTransportFactory.getTransport(client)


		See doc/specs/HeaderFormat.md for details of the wire format.

		"""

THRIFT-4621 Add THeader for Python #1583

THRIFT-4621 Add THeader for Python #1583

Conversation

spladug commented Aug 22, 2018 • edited

nsuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsuke Aug 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spladug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spladug commented Aug 27, 2018

jeking3 commented Aug 29, 2018

nsuke commented Sep 1, 2018

spladug commented Aug 22, 2018 •

edited

nsuke Aug 24, 2018 •

edited