Skip to content

v1.0.x manual upmerge#9145

Merged
nicolasnoble merged 67 commits intogrpc:masterfrom
nathanielmanistaatgoogle:v1.0.x-upmerge
Dec 19, 2016
Merged

v1.0.x manual upmerge#9145
nicolasnoble merged 67 commits intogrpc:masterfrom
nathanielmanistaatgoogle:v1.0.x-upmerge

Conversation

@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor

No description provided.

Muxi Yan and others added 30 commits November 10, 2016 17:25
This reverts commit 1079040.
Backport: Send RST_STREAM from client when it receives trailing metadata without the corresponding RST_STREAM
wait for write loop to finish at end of ruby read loop, on client side calls
The requirement that any created managed call must have operations
performed on it is obstructing proper handling of the case of
applications providing invalid invocation metadata. In such cases the
RPC is "over before it starts" when the very first call to
start_client_batch returns an error.
rescue GRPC::BadStatus => e
bad_status_code = e.code
rescue GRPC::ResourceExhausted
one_failed_as_unavailable = true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something is definitely off with this merge. This line changes from bad_status_code to one_failed_as_unavailable, but the check on line 425 still looks at bad_status_code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in conversation with @apolcyn.

expected_error_details = "metadata was invalid: %s" % metadata
with self.assertRaises(ValueError) as exception_context:
self._unary_unary(request, metadata=metadata)
self.assertIn(expected_error_details, str(exception_context.exception))
Copy link
Copy Markdown
Contributor

@ncteisen ncteisen Dec 16, 2016

Choose a reason for hiding this comment

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

Other option would be to swap expected_error_details with expected_error of type ValueError, then compare the str() of those two. But this works equally as well and probably has less unneeded overhead. Looks good!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I want to keep it in this form because I don't want to constrain the system under test to always raise a ValueError that always has a certain exact string. This test condition comes close to that, sure, but it at least buys a little flexibility for the system under test.

Manual changes:

 - Fixed use of Exception.message in _invalid_metadata_test.py

 - Fixed merge of one_failed_as_unavailable in rpc_server_spec.rb

 - Added "set -e" to generate_build_additions.sh
@apolcyn
Copy link
Copy Markdown
Contributor

apolcyn commented Dec 17, 2016

LGTM on ruby changes

@nathanielmanistaatgoogle
Copy link
Copy Markdown
Contributor Author

@nicolasnoble: tests are all green; please review and merge. 😄

@nicolasnoble
Copy link
Copy Markdown
Contributor

That looks good to me overall.

@nicolasnoble nicolasnoble merged commit f29de26 into grpc:master Dec 19, 2016
@nathanielmanistaatgoogle nathanielmanistaatgoogle deleted the v1.0.x-upmerge branch December 20, 2016 02:06
@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants