Python add support for utf-8 error messages by lidizheng · Pull Request #16946 · grpc/grpc

lidizheng · 2018-10-19T20:06:35Z

grpc-testing · 2018-10-19T20:49:57Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,238      Total (=)      1,997,238

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,642      Total (<)     11,021,645

 No significant differences in binary sizes

grpc-testing · 2018-10-19T21:23:22Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,238      Total (=)      1,997,238

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,641      Total (<)     11,021,646

 No significant differences in binary sizes

grpc-testing · 2018-10-19T22:04:42Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,238      Total (=)      1,997,238

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,762      Total (=)     11,021,762

 No significant differences in binary sizes

mehrdada

This is an area we really need tests for; ideally ones that failed previously.

lidizheng · 2018-10-20T17:03:24Z

The unit test I added will fail if my code are removed. And about the decoding part, I tried to decode to unicode object instead of passing the byte string to the user. It seems more consistent with previous logic.

lidizheng · 2018-10-20T17:08:53Z

You are right that I can't reproduce this client-side error... That's why I renamed the title to server-side.

Invalid encoding on b'\xc3'
Traceback (most recent call last):
  File "src/python/grpcio/grpc/_cython/_cygrpc/grpc_string.pyx.pxi", line 51, in grpc._cython.cygrpc._decode
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 0: unexpected end of data

grpc-testing · 2018-10-20T17:39:01Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,238      Total (=)      1,997,238

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,764      Total (>)     11,021,763

 No significant differences in binary sizes

grpc-testing · 2018-10-22T20:39:13Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,294      Total (=)      1,997,294

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,762      Total (<)     11,021,763

 No significant differences in binary sizes

grpc-testing · 2018-10-23T00:09:20Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,294      Total (=)      1,997,294

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,756      Total (>)     11,021,754

 No significant differences in binary sizes

grpc-testing · 2018-10-23T01:08:10Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,294      Total (=)      1,997,294

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,753      Total (<)     11,021,763

 No significant differences in binary sizes

grpc-testing · 2018-10-23T05:25:33Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,294      Total (=)      1,997,294

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,769      Total (>)     11,021,763

 No significant differences in binary sizes

grpc-testing · 2018-10-23T05:58:33Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,294      Total (=)      1,997,294

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,767      Total (>)     11,021,761

 No significant differences in binary sizes

lidizheng · 2018-10-23T17:16:41Z

@mehrdada Can you review the code? I added the unit test for UTF-8 error message, which will fail both the client side and server side without updated common.encode and common.decode.

euroelessar · 2018-10-23T17:36:03Z

Please also add a test for invalid UTF-8 messages, for example b'abc\x80\xd0\xaf' should be converted to something like u'abc\ufffd\u042f' or similar.

euroelessar · 2018-10-23T17:41:16Z

src/python/grpcio/grpc/_common.py


-def decode(b):
-    if isinstance(b, str):
+def decode(b):  #pylint: disable=inconsistent-return-statements


maybe smth simple like this?

if isinstance(b, bytes): return b.decode('utf-8', 'replace') return b

similar for def encode, just always encoding it as utf-8 should make a deal

If possible, I want to keep the message content. The replace will get rid of character that utf-8 can't understand. It will fail some cases, if people transmit pure binary data.

gRPC HTTP/2 spec explicitly states that message is an utf-8 message, so pure binary data is not an acceptable input (and therefore it should be sanitized, but not honored).
Please see https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses for more details.

It will be also consistent to grpc-go behavior, which currently sanitizes status messages by replacing invalid bytes sequences with the Unicode replacement character.

Please review the latest change. I have adopted your advices and added test cases.

lidizheng · 2018-10-23T21:24:57Z

@euroelessar Thanks for your suggestion. Since we don't need to worry about other codec, the logic is much simpler. And I added your test case to the unit test.

grpc-testing · 2018-10-23T22:04:51Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 1,997,274      Total (=)      1,997,274

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,021,701      Total (>)     11,021,696

 No significant differences in binary sizes

srini100 · 2018-10-24T18:31:15Z

@lidizheng, if this is going to affect metadata encoding/decoding, we should keep in mind that binary encoded metadata needs to be support based on this proposal https://github.com/grpc/proposal/blob/master/G1-true-binary-metadata.md.

lidizheng · 2018-10-26T18:14:12Z

@srini100
The encode/decode function for Metadata and error message (or status details or status message) is different. The encode/decode function in _common.py which I changed only affect four items:

url to listen/bind
exception message
method handler name
error message/status details. <---

As for the metadata in your concern, it is built in Cython layer which have another set of encode/decode functions in grpc_string.pyx.pxi. So, my change will not affect current logic of metadata at all.

And if we want to proceed to the unicode metadata... I guess we can open another issue for it.

grpc-testing · 2018-10-26T22:59:28Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-01T22:12:57Z

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

grpc-testing · 2018-11-01T23:45:54Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-01T23:58:22Z

[trickle] No significant performance differences

grpc-testing · 2018-11-02T00:26:16Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,015,578      Total (=)      2,015,578

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,116,197      Total (>)     11,116,193

 No significant differences in binary sizes

grpc-testing · 2018-11-02T00:38:41Z

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

grpc-testing · 2018-11-02T02:28:57Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-02T02:35:38Z

[trickle] No significant performance differences

grpc-testing · 2018-11-02T03:08:33Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,015,578      Total (=)      2,015,578

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,116,200      Total (>)     11,116,194

 No significant differences in binary sizes

grpc-testing · 2018-11-02T03:22:03Z

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

grpc-testing · 2018-11-02T23:35:16Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-02T23:46:51Z

[trickle] No significant performance differences

grpc-testing · 2018-11-03T00:16:38Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,016,505      Total (=)      2,016,505

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,123,078      Total (<)     11,123,080

 No significant differences in binary sizes

grpc-testing · 2018-11-03T00:26:45Z

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

lidizheng · 2018-11-05T21:02:00Z

@nicolasnoble Can you take a look at my latest changes? I added interop tests and they are passed in CI. But other tests that I didn't touch are failing. I tried to de-flake them by force-running multiple times, but they still fail...

* Both server and client should be fine with utf-8 error messages now * Adding an interop test: special status message

grpc-testing · 2018-11-05T21:37:30Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-11-05T21:49:42Z

[trickle] No significant performance differences

grpc-testing · 2018-11-05T22:18:17Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,616      Total (=)      2,020,616

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,180,943      Total (=)     11,180,943

 No significant differences in binary sizes

grpc-testing · 2018-11-05T22:29:58Z

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

tools/run_tests/run_interop_tests.py

lidizheng added lang/Python release notes: yes Indicates if PR needs to be in release notes labels Oct 19, 2018

lidizheng assigned mehrdada, nicolasnoble and lidizheng Oct 19, 2018

lidizheng requested review from mehrdada and nicolasnoble October 19, 2018 20:06

lidizheng changed the title ~~Add support for utf-8 error messages~~ Python server add support for utf-8 error messages Oct 19, 2018

mehrdada suggested changes Oct 20, 2018

View reviewed changes

lidizheng unassigned mehrdada and nicolasnoble Oct 23, 2018

euroelessar reviewed Oct 23, 2018

View reviewed changes

lidizheng added the kokoro:force-run label Nov 1, 2018

kokoro-team removed the kokoro:force-run label Nov 1, 2018

lidizheng added the kokoro:force-run label Nov 2, 2018

kokoro-team removed the kokoro:force-run label Nov 2, 2018

lidizheng added the kokoro:force-run label Nov 2, 2018

kokoro-team removed the kokoro:force-run label Nov 2, 2018

Add support for utf-8 error messages

b8a9989

* Both server and client should be fine with utf-8 error messages now * Adding an interop test: special status message

lidizheng force-pushed the py-utf-8 branch from efb4db8 to b8a9989 Compare November 5, 2018 21:32

gnossen approved these changes Nov 6, 2018

View reviewed changes

tools/run_tests/run_interop_tests.py Show resolved Hide resolved

lidizheng merged commit a04a928 into grpc:master Nov 6, 2018

ghost mentioned this pull request Dec 6, 2018

grpcio versions available: 1.17.0 Harmon758/Harmonbot#494

Closed

lock bot locked as resolved and limited conversation to collaborators Feb 4, 2019

Conversation

lidizheng commented Oct 19, 2018

Uh oh!

grpc-testing commented Oct 19, 2018

Uh oh!

grpc-testing commented Oct 19, 2018

Uh oh!

grpc-testing commented Oct 19, 2018

Uh oh!

mehrdada left a comment

Choose a reason for hiding this comment

Uh oh!

lidizheng commented Oct 20, 2018

Uh oh!

lidizheng commented Oct 20, 2018

Uh oh!

grpc-testing commented Oct 20, 2018

Uh oh!

grpc-testing commented Oct 22, 2018

Uh oh!

grpc-testing commented Oct 23, 2018

Uh oh!

grpc-testing commented Oct 23, 2018

Uh oh!

grpc-testing commented Oct 23, 2018

Uh oh!

grpc-testing commented Oct 23, 2018

Uh oh!

lidizheng commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

euroelessar commented Oct 23, 2018

Uh oh!

euroelessar Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

lidizheng Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

euroelessar Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

lidizheng Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

lidizheng commented Oct 23, 2018

Uh oh!

grpc-testing commented Oct 23, 2018

Uh oh!

srini100 commented Oct 24, 2018

Uh oh!

lidizheng commented Oct 26, 2018

Uh oh!

grpc-testing commented Oct 26, 2018

Uh oh!

grpc-testing commented Nov 1, 2018

Uh oh!

grpc-testing commented Nov 1, 2018

Uh oh!

grpc-testing commented Nov 1, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 2, 2018

Uh oh!

grpc-testing commented Nov 3, 2018

Uh oh!

grpc-testing commented Nov 3, 2018

lidizheng commented Oct 23, 2018 •

edited

Loading