Add check on return value from start_client_batch by ncteisen · Pull Request #8995 · grpc/grpc

ncteisen · 2016-12-07T23:34:41Z

The return value of client_start_batch was previously unchecked, which could cause a client to hang if it attempted to send a malformed header (see #8814).

There is now a check on the return of client_start_batch, and if there is a failure, then either an exception is raised, or an error rendezvous is returned depending on if the RPC was synchronous or asynchronous

nathanielmanistaatgoogle

Last time I saw a draft of this there were tests associated with it (right?); where'd they go?

nathanielmanistaatgoogle · 2016-12-08T21:28:28Z

src/python/grpcio/grpc/_channel.py

      condition.wait(timeout=remaining)

+def _check_call_error(call_error, metadata=None):
+  if call_error != cygrpc.CallError.ok:


Reduce the nesting by revising the control flow to be

if <one condition>: raise <one expression> elif <another condition>: raise <another expression>

.

nathanielmanistaatgoogle · 2016-12-08T21:29:21Z

src/python/grpcio/grpc/_channel.py

    else:
      condition.wait(timeout=remaining)

+def _check_call_error(call_error, metadata=None):


Keyword arguments generally are not appropriate for private methods - make metadata positional?

nathanielmanistaatgoogle · 2016-12-08T21:33:20Z

src/python/grpcio/grpc/_channel.py

+            event_handler)
+        if call_error != cygrpc.CallError.ok:
+          return _Rendezvous(
+              _call_error_RPCstate(call_error, metadata), None, None, deadline)


Normally I'm the guy who's totally against all mutation, but... I think this code is easier to reason about if there remains exactly one _RPCState per RPC. I think you should change _call_error_RPCstate to mutate the _RPCState passed to it rather than create a new instance.

ncteisen · 2016-12-08T23:57:44Z

Addressed comments on _channel.py and included my testing file in the commit.

The testing file is pretty hacked up at this point. I can make it cleaner, and move it to the unit test folder if it deserves to be there.

nathanielmanistaatgoogle · 2016-12-09T00:10:54Z

This change is significant enough that it must have its own test coverage. Please clean the test and put it where it's supposed to be.

ncteisen · 2016-12-13T21:25:55Z

Added a unit test for this new behavior

nathanielmanistaatgoogle

Consider running pylint over your changed files - it'll probably spam you with complaints about the two-space indentation (can't wait!) but everything starting with E, everything starting with W, and most things starting with C should be actionable.

nathanielmanistaatgoogle · 2016-12-13T21:31:31Z

src/python/grpcio/grpc/_channel.py

      condition.wait(timeout=remaining)

+def _check_call_error(call_error, metadata):
+  if call_error == cygrpc.CallError.ok:


Why this rather than

if <one error condition>: raise <one error> elif <another error condition>: raise <another error>

?

nathanielmanistaatgoogle · 2016-12-13T21:33:39Z

src/python/grpcio/grpc/_channel.py

+  elif call_error == cygrpc.CallError.invalid_metadata:
+    raise ValueError("metadata was invalid: %s" % metadata)
+  else:
+    raise ValueError('Internal gRPC CallError %d. Please report to '


Avoid implicit string concatenation whenever you can.

If our issue tracker URL is going to appear twice in program source, define a constant for it?

nathanielmanistaatgoogle · 2016-12-13T21:34:07Z

src/python/grpcio/grpc/_channel.py

+  if call_error == cygrpc.CallError.ok:
+    return
+  elif call_error == cygrpc.CallError.invalid_metadata:
+    raise ValueError("metadata was invalid: %s" % metadata)


Maintain consistent use of single-quote as the string delimiter throughout this file.

nathanielmanistaatgoogle · 2016-12-13T21:41:20Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    metadata = (('InVaLiD', 'UnaryRequestBlockingUnaryResponse'),)
+    expected_error = ValueError("metadata was invalid: %s" % metadata)
+    multi_callable = _unary_unary_multi_callable(self._channel)
+    with self.assertRaises(ValueError) as cm:


Avoid abbreviations for the same reason that single-letter identifiers are forbidden by the style guide. This should probably be named exception_context.

nathanielmanistaatgoogle · 2016-12-13T21:42:11Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+  def testUnaryRequestBlockingUnaryResponse(self):
+    request = b'\x07\x08'
+    metadata = (('InVaLiD', 'UnaryRequestBlockingUnaryResponse'),)
+    expected_error = ValueError("metadata was invalid: %s" % metadata)


You don't really use this ValueError instance except for the value of its message attribute. Why create it?

nathanielmanistaatgoogle · 2016-12-13T21:43:49Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    expected_error_details = "metadata was invalid: %s" % metadata
+    multi_callable = _unary_unary_multi_callable(self._channel)
+    with self.assertRaises(Exception) as cm:
+      response_future = multi_callable.future(request, metadata=metadata)


Pull this out of the with so that the test has the effect of asserting that execution of this line does not raise an exception.

nathanielmanistaatgoogle · 2016-12-13T21:44:12Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    metadata = (('InVaLiD', 'UnaryRequestFutureUnaryResponse'),)
+    expected_error_details = "metadata was invalid: %s" % metadata
+    multi_callable = _unary_unary_multi_callable(self._channel)
+    with self.assertRaises(Exception) as cm:


No more specific exception class? Exception is very broad.

nathanielmanistaatgoogle · 2016-12-13T21:45:10Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    with self.assertRaises(Exception) as cm:
+      response_future = multi_callable.future(request, metadata=metadata)
+      response = response_future.result()
+    self.assertEqual(cm.exception.details(), expected_error_details)


Please also assert that response_future.code()'s return value is as expected (and maybe response_future.details()?

nathanielmanistaatgoogle · 2016-12-13T21:46:18Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    multi_callable = _unary_unary_multi_callable(self._channel)
+    with self.assertRaises(ValueError) as cm:
+      response = multi_callable(request, metadata=metadata)
+    self.assertEqual(cm.exception.message, expected_error.message)


Please maintain the gRPC-Python-codebase-wide convention of self.assertEqual(<expected>, <actual>) over self.assertEqual(<actual>, <expected>).

nathanielmanistaatgoogle · 2016-12-13T21:48:08Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    request = b'\x07\x08'
+    metadata = (('InVaLiD', 'UnaryRequestBlockingUnaryResponse'),)
+    expected_error = ValueError("metadata was invalid: %s" % metadata)
+    multi_callable = _unary_unary_multi_callable(self._channel)


Kind of an arbitrary matter of taste but consider moving the instantiation of the MultiCallable objects to setUp rather than having one-per-test-method:

self._unary_unary = <...> self._unary_stream = <...> <...>

.

nathanielmanistaatgoogle · 2016-12-13T22:07:56Z

Does the new test need to be recorded in tests.json?

ncteisen · 2016-12-14T03:05:53Z

Addressed all comments. Added the test to tests.json

nathanielmanistaatgoogle · 2016-12-14T21:40:04Z

src/python/grpcio/grpc/_channel.py

    else:
      condition.wait(timeout=remaining)

+_INTERNAL_CALLERROR_MESSAGE = (


Underscore between CALL and ERROR (corresponding to and due to the way cygrpc.CallError is capitalized).

nathanielmanistaatgoogle · 2016-12-14T21:41:01Z

src/python/grpcio/grpc/_channel.py

    else:
      condition.wait(timeout=remaining)

+_INTERNAL_CALLERROR_MESSAGE = (


Since this contains a % to be replaced it is a message format rather than a message. _INTERNAL_CALL_ERROR_MESSAGE_FORMAT?

nathanielmanistaatgoogle · 2016-12-14T21:42:45Z

src/python/grpcio/grpc/_channel.py

      condition.wait(timeout=remaining)

+_INTERNAL_CALLERROR_MESSAGE = (
+    'Internal gRPC CallError %d. ' +


And just plain old ordinary "call error" here, since what semantically matters in this user-visible (or developer-visible) string is not the specific CallError class name.

nathanielmanistaatgoogle · 2016-12-14T21:44:56Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    request = b'\x07\x08'
+    metadata = (('InVaLiD', 'UnaryRequestBlockingUnaryResponse'),)
+    expected_error_details = "metadata was invalid: %s" % metadata
+    with self.assertRaises(ValueError) as exception_ctx:


Avoid abbreviations as well as initialisms and acronyms - this should be exception_context.

nathanielmanistaatgoogle · 2016-12-14T21:46:24Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    metadata = (('InVaLiD', 'UnaryRequestBlockingUnaryResponse'),)
+    expected_error_details = "metadata was invalid: %s" % metadata
+    with self.assertRaises(ValueError) as exception_ctx:
+      response = self._unary_unary(request, metadata=metadata)


Did you try running pylint? Did it alert you to this local field being assigned but never accessed?

I'd say drop the field, but if you think the test is more clear with it included call it unused_response.

nathanielmanistaatgoogle · 2016-12-14T21:48:57Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    self.assertEqual(expected_error_details, exception_ctx.exception.message)
+
+  def testStreamRequestFutureUnaryResponse(self):
+    requests = tuple(b'\x07\x08' for _ in range(test_constants.STREAM_LENGTH))


Why not just

request_iterator = (b'\x07\x08' for _ in range(test_constants.STREAM_LENGTH))

rather than these two lines? You should only need to create the sequence value if you need to iterate over its elements more than once.

ncteisen · 2016-12-15T00:06:56Z

Addressed all comments. Now pylint only complains about the indentation and missing docstring, but the all the other unit tests don't have docstrings, so I assumed that was ok?

nathanielmanistaatgoogle

Regarding pylint: yes.

Sorry for missing these two small but important comments in the last round.

nathanielmanistaatgoogle · 2016-12-15T00:50:19Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    expected_error_details = "metadata was invalid: %s" % metadata
+    response_iterator = self._unary_stream(request, metadata=metadata)
+    with self.assertRaises(grpc.RpcError) as exception_context:
+      tuple(response_iterator)


By changing this from tuple to next you again narrow the execution from which you are asserting that an exception must rise (from "an exception arises from consuming the entire stream of responses (including the end-of-stream indicator)" to "an exception arises from consuming the very first response in the response stream").

nathanielmanistaatgoogle · 2016-12-15T00:52:34Z

src/python/grpcio_tests/tests/unit/_invalid_metadata_test.py

+    expected_error_details = "metadata was invalid: %s" % metadata
+    response_iterator = self._stream_stream(request_iterator, metadata=metadata)
+    with self.assertRaises(grpc.RpcError) as exception_context:
+      tuple(response_iterator)


ncteisen · 2016-12-15T18:07:59Z

Done

nathanielmanistaatgoogle

Thank you!

ncteisen added kind/enhancement lang/Python labels Dec 7, 2016

ncteisen requested a review from nathanielmanistaatgoogle December 7, 2016 23:34

googlebot added the cla: yes label Dec 7, 2016

nathanielmanistaatgoogle suggested changes Dec 8, 2016

View reviewed changes

ncteisen force-pushed the python_call_batch branch from c65ae8b to 92a1a7e Compare December 13, 2016 21:24

nathanielmanistaatgoogle suggested changes Dec 13, 2016

View reviewed changes

ncteisen force-pushed the python_call_batch branch from 92a1a7e to 962592d Compare December 14, 2016 03:05

nathanielmanistaatgoogle suggested changes Dec 14, 2016

View reviewed changes

nathanielmanistaatgoogle self-assigned this Dec 14, 2016

ncteisen force-pushed the python_call_batch branch from 962592d to 12c3493 Compare December 15, 2016 00:05

nathanielmanistaatgoogle suggested changes Dec 15, 2016

View reviewed changes

Add check on return value from start_client_batch

61b8c89

ncteisen force-pushed the python_call_batch branch from 12c3493 to 61b8c89 Compare December 15, 2016 18:07

nathanielmanistaatgoogle approved these changes Dec 15, 2016

View reviewed changes

ncteisen merged commit 3fa20cf into grpc:v1.0.x Dec 15, 2016

ncteisen deleted the python_call_batch branch December 15, 2016 19:20

ncteisen mentioned this pull request Dec 20, 2016

Python client is hung when sending a header in upper-case #8814

Closed

nathanielmanistaatgoogle mentioned this pull request Jan 18, 2017

Error reporting: invalid metadata #3042

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019

Conversation

ncteisen commented Dec 7, 2016

Uh oh!

nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncteisen commented Dec 8, 2016

Uh oh!

nathanielmanistaatgoogle commented Dec 9, 2016

Uh oh!

ncteisen commented Dec 13, 2016

Uh oh!

nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanielmanistaatgoogle commented Dec 13, 2016

Uh oh!

ncteisen commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncteisen commented Dec 15, 2016

Uh oh!

nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncteisen commented Dec 15, 2016

Uh oh!

nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants