-
Notifications
You must be signed in to change notification settings - Fork 5.5k
http_filter: add addEncodedTrailers and addDecodedTrailers #3980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
34bd3d1
688bc4d
e0b74f3
acfa3d8
baee8aa
42443b0
52c5780
8495e41
36103a1
f6ca600
483b48b
5412bb8
cd25726
c3a900b
90c6233
3667d6a
7637fa3
f2b6210
815a07b
2321031
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,6 +190,18 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { | |
| */ | ||
| virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE; | ||
|
|
||
| /** | ||
| * Adds decoded trailers. May only be called in decodeData when end_stream is set to true or in | ||
| * decodeTrailers. If called in any other context, std::logic_error will be thrown. | ||
| * | ||
| * When called in decodeData, the trailers map will be initialized to an empty map and returned by | ||
| * reference. Calling it more than once is invalid and will result in std::logic_error being thrown. | ||
| * | ||
| * When called in decodeTrailers it will simply return the trailers (same data as passed in the | ||
| * parameter). | ||
| */ | ||
| virtual HeaderMap& addDecodedTrailers() PURE; | ||
|
|
||
| /** | ||
| * Create a locally generated response using the provided response_code and body_text parameters. | ||
| * If the request was a gRPC request the local reply will be encoded as a gRPC response with a 200 | ||
|
|
@@ -395,6 +407,18 @@ class StreamEncoderFilterCallbacks : public virtual StreamFilterCallbacks { | |
| */ | ||
| virtual void addEncodedData(Buffer::Instance& data, bool streaming_filter) PURE; | ||
|
|
||
| /** | ||
| * Adds encoded trailers. May only be called in encodeData when end_stream is set to true or in | ||
| * encodeTrailers. If called in any other context, std::logic_error will be thrown. | ||
| * | ||
| * When called in encodeData, the trailers map will be initialized to an empty map and returned by | ||
| * reference. Calling it more than once is invalid and will result in std::logic_error being thrown. | ||
| * | ||
| * When called in encodeTrailers it will simply return the trailers (same data as passed in the | ||
| * parameter). | ||
| */ | ||
| virtual HeaderMap& addEncodedTrailers() PURE; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unsure what the best signature was here - it seems like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this work if the target stream protocol is not able to support trailers?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I understand the http/1.1 codec will just ignore the trailers: https://github.com/envoyproxy/envoy/blob/master/source/common/http/http1/codec_impl.cc#L148
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we should just pass a HeaderMapPtr via move? This would just overwrite existing trailers? It would be uyp to the user to figure out if trailers already exist? Thoughts?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In that case you'll leaking HeaderMapImpl into filter codes and let filter create the HeaderMapPtr. Can we somehow (e.g. ASSERT) make sure this can be only called after or in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will want to make sure for any protocol which doesn't support trailers we don't then lose the end_stream=true passing through the pipeline, but that's what tests are for :-) |
||
|
|
||
| /** | ||
| * Called when an encoder filter goes over its high watermark. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -775,15 +775,45 @@ void ConnectionManagerImpl::ActiveStream::decodeData(ActiveStreamDecoderFilter* | |
|
|
||
| for (; entry != decoder_filters_.end(); entry++) { | ||
| ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeData)); | ||
|
|
||
| // We check the request_trailers_ pointer here in case addDecodedTrailers | ||
| // is called in decodeData - at which point we communicate to the filter | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "is called in decodeData during a previous filter invocation, at which point we communicate to the current and future filters that the stream has not yet ended." |
||
| // that the stream has not yet ended. | ||
| bool end_stream_no_trailers = end_stream && !request_trailers_; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind adding a comment that we check the state of end stream inside the loop in case a filter adds trailers? |
||
| if (end_stream_no_trailers) { | ||
| state_.filter_call_state_ |= FilterCallState::LastDataFrame; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd argue changing HCM state probably bumps this up to a medium-risk change.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does HCM stand for in this context?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Http Connection Manager (HCM) |
||
| state_.filter_call_state_ |= FilterCallState::DecodeData; | ||
| FilterDataStatus status = (*entry)->handle_->decodeData(data, end_stream); | ||
| FilterDataStatus status = (*entry)->handle_->decodeData(data, end_stream_no_trailers); | ||
| state_.filter_call_state_ &= ~FilterCallState::DecodeData; | ||
| if (end_stream_no_trailers) { | ||
| state_.filter_call_state_ &= ~FilterCallState::LastDataFrame; | ||
| } | ||
| ENVOY_STREAM_LOG(trace, "decode data called: filter={} status={}", *this, | ||
| static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status)); | ||
| if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.decoder_filters_streaming_)) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // If trailers were adding during decodeData we need to trigger decodeTrailers in order | ||
| // to allow filters to process the trailers. | ||
| if (end_stream && request_trailers_) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again let's comment this is explicitly for the case that the remote side didn't send trailers but a filter added them. |
||
| decodeTrailers(filter, *request_trailers_); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we need to actually only send trailers to the filter that set the trailer and filters after it? I think we need to actually track which filter added the trailers... (See how this is handled in the headers/data case). |
||
| } | ||
| } | ||
|
|
||
| HeaderMap& ConnectionManagerImpl::ActiveStream::addDecodedTrailers() { | ||
| if (state_.filter_call_state_ & FilterCallState::LastDataFrame) { | ||
| if (request_trailers_) { | ||
| throw std::logic_error("decodedTrailers added more than once"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what will catch these exceptions? Perhaps they should be CodecProtocolException so that the existing codec code will catch them and terminate the connection?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I picked a non-EnvoyException exception as per @mattklein123 suggestion in a previous comment (#3980 (comment)). If we want this to fail more gracefully then I'm happy to do so, just want to make sure we're all in agreement before switching over.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this is the right exception type so that we don't catch it and crash/core dump. I think a logic error like this should be caught during development time and should be very obvious?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, to be clear, you think this exception should not be caught? I grant with our current example filters it should be fairly obvious, but once folks have custom filters which do non-trivial async work which may have timing invariants, and/or we make the pipeline more complicated with internal redirects, I think it gets less obvious. I'd strongly prefer developer errors cause disconnects and not crashes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think it should not be caught. If you catch this exception you will need to add stats and logging to make it obvious what is happening otherwise it will be incredibly confusing. Basically, it's a bunch of error handling and operational work that IMO should not happen in practice. If there is disagreement on using an exception for this I would switch to RELEASE_ASSERT and make it crash.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the tl;dr here is that I should be using ASSERTs instead of exceptions, deferring better error handling to a future change? @alyssawilk Given this, would you still want integration tests? It'd only test the happy path (unless we can test asserts somehow?), but it might still be worth having?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tl;dr sounds right! Yeah, I think given this we're just testing the happy path. One theoretically could do a debug death test for broken filters but even I think that'd be overkill :-)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool I'll look into it. Could you point me to some integration tests I could extend/based mine off to cover these changes? Out of curiosity: what would a "debug death test" look like?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've got a bunch of tests which add filters (see config_helper_.addFilter()) so what we'll need to do is add a test (probably in http_integration.cc and then called from the various http[2][_upstream]integration_test.cc) which does makeRequestWithBody() and then verifies header/body/data get proxied downstream and then the flip side, upstream_request->encodeHeaders() and encodeData with trailers being received by H2 downstream. The trickier bit is we don't have a sample filter which does this, so you'll need to add a test filter which always adds trailers. In the long run I'd like a test filter factory which creates MockStreamFilter which we can use that for all sorts of custom test behavior but I think that's out of scope for this PR :-) If you do the simple one I'll hopefully get some free time to convert it to the fancy version later in. Actually it occurs to me thinking about makeRequestWithBody vs makeHeaderOnlyRequest.... we allow adding trailers on decodeData(end_stream = true) but should we also allow it in decodeHeaders (end_stream=true)? It seems more consistent for H2 but if we're really only wanting this for gRPC and we think we won't hit gRPC with no body where we want to add trailers, I'd be OK documenting it as a known limitation someone else can fix when/if they need it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the pointers, I'll try to get some tests set up. I can't really think of a use case of adding trailers to request/response with no DATA frames (how is that even different from just adding additional headers?). I'd prefer to to just leave it as it is right now. Adding it in later should be fairly straightforward once all the unit tests/integration tests are in place. |
||
| } | ||
|
|
||
| request_trailers_ = std::make_unique<HeaderMapImpl>(); | ||
| return *request_trailers_; | ||
| } else { | ||
| throw std::logic_error("addDecodedTrailers called in invalid context"); | ||
| } | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::addDecodedData(ActiveStreamDecoderFilter& filter, | ||
|
|
@@ -1058,6 +1088,24 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte | |
| } | ||
| } | ||
|
|
||
| HeaderMap& ConnectionManagerImpl::ActiveStream::addEncodedTrailers() { | ||
| if (state_.filter_call_state_ & FilterCallState::LastDataFrame) { | ||
| if (response_trailers_) { | ||
| throw std::logic_error("encodedTrailers added more than once"); | ||
| } | ||
|
|
||
| // local_complete_ is set to true at the start of encodeData(..., true), but since | ||
| // we've now added trailers we have to undo this to prevent encodeTrailers from | ||
| // blowing up when it assert on local_complete_ | ||
| state_.local_complete_ = false; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was necessary because
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's worth a comment in the PR it's worth a comment in the code :-) |
||
|
|
||
| response_trailers_ = std::make_unique<HeaderMapImpl>(); | ||
| return *response_trailers_; | ||
| } else { | ||
| throw std::logic_error("encodedTrailers called in invalid context"); | ||
| } | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::addEncodedData(ActiveStreamEncoderFilter& filter, | ||
| Buffer::Instance& data, bool streaming) { | ||
| if (state_.filter_call_state_ == 0 || | ||
|
|
@@ -1085,9 +1133,20 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter* | |
| std::list<ActiveStreamEncoderFilterPtr>::iterator entry = commonEncodePrefix(filter, end_stream); | ||
| for (; entry != encoder_filters_.end(); entry++) { | ||
| ASSERT(!(state_.filter_call_state_ & FilterCallState::EncodeData)); | ||
|
|
||
| // We check the request_trailers_ pointer here in case addEncodedTrailers | ||
| // is called in encodeData - at which point we communicate to the filter | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about the comment and the local variable. |
||
| // that the stream has not yet ended. | ||
| bool end_stream_no_trailers = end_stream && !response_trailers_; | ||
| state_.filter_call_state_ |= FilterCallState::EncodeData; | ||
| FilterDataStatus status = (*entry)->handle_->encodeData(data, end_stream); | ||
| if (end_stream_no_trailers) { | ||
| state_.filter_call_state_ |= FilterCallState::LastDataFrame; | ||
| } | ||
| FilterDataStatus status = (*entry)->handle_->encodeData(data, end_stream_no_trailers); | ||
| state_.filter_call_state_ &= ~FilterCallState::EncodeData; | ||
| if (end_stream_no_trailers) { | ||
| state_.filter_call_state_ &= ~FilterCallState::LastDataFrame; | ||
| } | ||
| ENVOY_STREAM_LOG(trace, "encode data called: filter={} status={}", *this, | ||
| static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status)); | ||
| if (!(*entry)->commonHandleAfterDataCallback(status, data, state_.encoder_filters_streaming_)) { | ||
|
|
@@ -1099,8 +1158,16 @@ void ConnectionManagerImpl::ActiveStream::encodeData(ActiveStreamEncoderFilter* | |
| end_stream); | ||
|
|
||
| request_info_.addBytesSent(data.length()); | ||
| response_encoder_->encodeData(data, end_stream); | ||
| maybeEndEncode(end_stream); | ||
|
|
||
| // If trailers were adding during encodeData we need to trigger decodeTrailers in order | ||
| // to allow filters to process the trailers. | ||
| if (end_stream && response_trailers_) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little confused about the semantics here. When is a filter expected to add trailers for the first time? Do we want to support setting trailers during the final data frame where end_stream was otherwise true? I don't think we want to support that and we probably do? Also where is the parallel to this code on the decoding side?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens right now (since you can override trailers whenever)
3 is definitely confusing, so I'd be happy to prevent that. The question to me that remains is whether we want trailers to be written out at the end of What steered me away from that originally was that was that it doesn't seem like So after thinking about it some, how about this: This does mean we get both a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think this is more on the right track.
I don't think this is going to work as it will completely confuse filters that do things when end_stream is true. I think this is the behavior you want:
I don't think this should be too hard to implement and should cleanly cover both encode/decode. Does that make sense? For the error checking side of things, IMO I would probably just block trailers being added in any context other than decodeData(..., true). Does it make sense anywhere else? The otehr thing to consider is what if someone calls this when a filter has been paused and then calls continue? Will it work correctly?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that makes sense for the most part. One question:
Do you mean
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No I meant what I said, that I think the only valid context to add trailers should be within a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that makes perfect sense. Thanks for clarifying. |
||
| response_encoder_->encodeData(data, false); | ||
| encodeTrailers(filter, *response_trailers_); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here about keeping track of which filter added the trailers? As an aside, I'm a little confused as to why the logic in encodeHeaders() does not mirror decodeHeaders() WRT to handling continue/last filter. (And yes I know I wrote this code!) I can look into this more tomorrow or Friday, but if you feel like taking a look that would be appreciated.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into it a little bit: adding the same last filter check to encodHeaders causes a test failure in which presumably comes from hitting this code more than once (towards the end of The test that's failing is Unclear whether this is why we don't special case the last filter in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thanks for looking. I can take a look tomorrow or this weekend more. I wouldn't worry about it for now but it seemed strange to me that it wasn't symmetrical and I couldn't remember why. |
||
| } else { | ||
| response_encoder_->encodeData(data, end_stream); | ||
| maybeEndEncode(end_stream); | ||
| } | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilter* filter, | ||
|
|
@@ -1380,6 +1447,10 @@ Buffer::WatermarkBufferPtr ConnectionManagerImpl::ActiveStreamDecoderFilter::cre | |
| return buffer; | ||
| } | ||
|
|
||
| HeaderMap& ConnectionManagerImpl::ActiveStreamDecoderFilter::addDecodedTrailers() { | ||
| return parent_.addDecodedTrailers(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamDecoderFilter::addDecodedData(Buffer::Instance& data, | ||
| bool streaming) { | ||
| parent_.addDecodedData(*this, data, streaming); | ||
|
|
@@ -1473,6 +1544,10 @@ void ConnectionManagerImpl::ActiveStreamEncoderFilter::addEncodedData(Buffer::In | |
| return parent_.addEncodedData(*this, data, streaming); | ||
| } | ||
|
|
||
| HeaderMap& ConnectionManagerImpl::ActiveStreamEncoderFilter::addEncodedTrailers() { | ||
| return parent_.addEncodedTrailers(); | ||
| } | ||
|
|
||
| void ConnectionManagerImpl::ActiveStreamEncoderFilter:: | ||
| onEncoderFilterAboveWriteBufferHighWatermark() { | ||
| ENVOY_STREAM_LOG(debug, "Disabling upstream stream due to filter callbacks.", parent_); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,6 +162,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
|
|
||
| // Http::StreamDecoderFilterCallbacks | ||
| void addDecodedData(Buffer::Instance& data, bool streaming) override; | ||
| HeaderMap& addDecodedTrailers() override; | ||
| void continueDecoding() override; | ||
| const Buffer::Instance* decodingBuffer() override { | ||
| return parent_.buffered_request_data_.get(); | ||
|
|
@@ -229,6 +230,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
|
|
||
| // Http::StreamEncoderFilterCallbacks | ||
| void addEncodedData(Buffer::Instance& data, bool streaming) override; | ||
| HeaderMap& addEncodedTrailers() override; | ||
| void onEncoderFilterAboveWriteBufferHighWatermark() override; | ||
| void onEncoderFilterBelowWriteBufferLowWatermark() override; | ||
| void setEncoderBufferLimit(uint32_t limit) override { parent_.setBufferLimit(limit); } | ||
|
|
@@ -267,11 +269,13 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_stream); | ||
| const Network::Connection* connection(); | ||
| void addDecodedData(ActiveStreamDecoderFilter& filter, Buffer::Instance& data, bool streaming); | ||
| HeaderMap& addDecodedTrailers(); | ||
| void decodeHeaders(ActiveStreamDecoderFilter* filter, HeaderMap& headers, bool end_stream); | ||
| void decodeData(ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream); | ||
| void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers); | ||
| void maybeEndDecode(bool end_stream); | ||
| void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); | ||
| HeaderMap& addEncodedTrailers(); | ||
| void sendLocalReply(bool is_grpc_request, Code code, const std::string& body, | ||
| std::function<void(HeaderMap& headers)> modify_headers); | ||
| void encode100ContinueHeaders(ActiveStreamEncoderFilter* filter, HeaderMap& headers); | ||
|
|
@@ -339,6 +343,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>, | |
| // to verify we do not encode100Continue headers more than once per | ||
| // filter. | ||
| static constexpr uint32_t Encode100ContinueHeaders = 0x40; | ||
| // Used to indicate that we're processing the final [en|de]Code frame, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| // i.e. end_stream = true | ||
| static constexpr uint32_t LastDataFrame = 0x80; | ||
| }; | ||
| // clang-format on | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call this under decodeTrailers? isn't decodeTrailers the wrong state (not LastDataFrame) and would have request_trailers_ non-null (exception either way)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure if we're consistent about this but can you add a javadoc style @throws here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right - I was misunderstanding when
request_trailers_was set in the normal flow. The test failures were due to exactly this.I'll add in the
@throwsas well.