Publish stream events before calling RequestLogBuilder
#2832
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation:
We currently call `RequestLogBuilder` methods even before publishing
stream events (`StreamMesssage.write/close/abort()`) in some places,
such as `HttpResponseDecoder`. This can be a problem because the
callbacks added to the `RequestLog` may be invoked even before any
stream events are published.
For example, `HttpResponseDecoder` may fail to publish the received
`ResponseHeaders` when a `RequestLog` callback aborts the response
even before the publication, leading to the following exception:
io.netty.handler.codec.http2.Http2Exception: failed to consume a HEADERS frame
at io.netty.handler.codec.http2.Http2Exception.connectionError(Http2Exception.java:117)
at com.linecorp.armeria.client.Http2ResponseDecoder.onHeadersRead(Http2ResponseDecoder.java:206)
at com.linecorp.armeria.client.Http2ResponseDecoder.onHeadersRead(Http2ResponseDecoder.java:219)
...
Caused by: com.linecorp.armeria.common.stream.ClosedStreamException: null
at com.linecorp.armeria.common.stream.ClosedStreamException.get(ClosedStreamException.java:37)
at com.linecorp.armeria.common.stream.StreamWriter.write(StreamWriter.java:72)
at com.linecorp.armeria.client.Http2ResponseDecoder.onHeadersRead(Http2ResponseDecoder.java:203)
... 40 common frames omitted
Modifications:
- Add a generalized mechanism to defer any `RequestLogProperty`:
- `isDeferred(RequestLogProperty)`
- `defer(RequestLogProperty)`
- `defer*Content*()` and `isDefer*Content*Set()` have been deprecated.
- Defer setting some log properties before publishing stream events,
so that they are recorded after the publication.
- Miscellaneous:
- Do not warn unnecessarily when propagating content previews in
`DefaultRequestLog.propagate*()`.
- Make `HttpClientMaxConcurrentStreamTest.exceededMaxStreamsPropagatesFailureCorrectly()`
less flaky.
- Optimize `DefaultRequestLog.isAvailable()`
Result:
- No more `Http2Exception: failed to consume a HEADERS frame` error.
- No more `You tried to set the content preview twice` warning.
- Fixes line#2830
- (Deprecation) `RequestLogBuilder.defer*Content*()` and
`isDefer*Content*Set()` have been deprecated in favor of
`defer(RequestLogProperty)` and `isDeferred(RequestLogProperty)`.
trustin
commented
Jun 24, 2020
| final HttpRequest req = request(); | ||
| if (req != null) { | ||
| autoFillSchemeAndAuthority(); | ||
| req.abort(cause); | ||
| req.abort(wrapped); |
There was a problem hiding this comment.
Not sure why it was cause before. 🤔
There was a problem hiding this comment.
I think it didn't do any harm anyways. 😆
First appeared at: https://github.com/line/armeria/pull/2248/files#diff-d437ddb5e0b4572dba776949bacf9becR233
hexoul
reviewed
Jun 24, 2020
Codecov Report
@@ Coverage Diff @@
## master #2832 +/- ##
============================================
- Coverage 72.79% 72.69% -0.11%
+ Complexity 12055 12039 -16
============================================
Files 1069 1068 -1
Lines 46848 46881 +33
Branches 5866 5868 +2
============================================
- Hits 34105 34081 -24
- Misses 9736 9793 +57
Partials 3007 3007 Continue to review full report at Codecov.
|
minwoox
approved these changes
Jun 24, 2020
| requireNonNull(properties, "properties"); | ||
| final int flags = interestedFlags(properties); | ||
| checkArgument(flags != 0, "properties is empty."); | ||
| return isAvailable(flags); | ||
| } | ||
|
|
||
| private boolean isAvailable(int interestedFlags) { |
There was a problem hiding this comment.
nit: can remove this method and just call hasInterestedFlags directly?
There was a problem hiding this comment.
I guess it's better keeping it for readability - isAvailable(interestedFlags) vs. hasInterestedFlags(this.flags, interestedFlags).
| warnedSettingContentPreviewTwice = true; | ||
| logger.warn("You tried to set the content preview twice: {} " + | ||
| logger.warn("You tried to set the response content preview twice: {} " + |
ikhoon
approved these changes
Jun 26, 2020
| if (!isDeferRequestContentSet() || | ||
| (isDeferRequestContentSet() && isAvailable(RequestLogProperty.REQUEST_CONTENT))) { | ||
| if (!hasInterestedFlags(deferredFlags, RequestLogProperty.REQUEST_CONTENT) || | ||
| isAvailable(RequestLogProperty.REQUEST_CONTENT)) { |
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
Motivation:
We currently call `RequestLogBuilder` methods even before publishing
stream events (`StreamMesssage.write/close/abort()`) in some places,
such as `HttpResponseDecoder`. This can be a problem because the
callbacks added to the `RequestLog` may be invoked even before any
stream events are published.
For example, `HttpResponseDecoder` may fail to publish the received
`ResponseHeaders` when a `RequestLog` callback aborts the response
even before the publication, leading to the following exception:
io.netty.handler.codec.http2.Http2Exception: failed to consume a HEADERS frame
at io.netty.handler.codec.http2.Http2Exception.connectionError(Http2Exception.java:117)
at com.linecorp.armeria.client.Http2ResponseDecoder.onHeadersRead(Http2ResponseDecoder.java:206)
at com.linecorp.armeria.client.Http2ResponseDecoder.onHeadersRead(Http2ResponseDecoder.java:219)
...
Caused by: com.linecorp.armeria.common.stream.ClosedStreamException: null
at com.linecorp.armeria.common.stream.ClosedStreamException.get(ClosedStreamException.java:37)
at com.linecorp.armeria.common.stream.StreamWriter.write(StreamWriter.java:72)
at com.linecorp.armeria.client.Http2ResponseDecoder.onHeadersRead(Http2ResponseDecoder.java:203)
... 40 common frames omitted
Modifications:
- Add a generalized mechanism to defer any `RequestLogProperty`:
- `isDeferred(RequestLogProperty)`
- `defer(RequestLogProperty)`
- `defer*Content*()` and `isDefer*Content*Set()` have been deprecated.
- Defer setting some log properties before publishing stream events,
so that they are recorded after the publication.
- Miscellaneous:
- Do not warn unnecessarily when propagating content previews in
`DefaultRequestLog.propagate*()`.
- Make `HttpClientMaxConcurrentStreamTest.exceededMaxStreamsPropagatesFailureCorrectly()`
less flaky.
- Optimize `DefaultRequestLog.isAvailable()`
Result:
- No more `Http2Exception: failed to consume a HEADERS frame` error.
- No more `You tried to set the content preview twice` warning.
- Fixes line#2830
- Slightly faster `RequestLogAccess.isAvailable()` implementation.
- (Deprecation) `RequestLogBuilder.defer*Content*()` and
`isDefer*Content*Set()` have been deprecated in favor of
`defer(RequestLogProperty)` and `isDeferred(RequestLogProperty)`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
We currently call
RequestLogBuildermethods even before publishingstream events (
StreamMesssage.write/close/abort()) in some places,such as
HttpResponseDecoder. This can be a problem because thecallbacks added to the
RequestLogmay be invoked even before anystream events are published.
For example,
HttpResponseDecodermay fail to publish the receivedResponseHeaderswhen aRequestLogcallback aborts the responseeven before the publication, leading to the following exception:
Modifications:
RequestLogProperty:isDeferred(RequestLogProperty)defer(RequestLogProperty)defer*Content*()andisDefer*Content*Set()have been deprecated.so that they are recorded after the publication.
DefaultRequestLog.propagate*().HttpClientMaxConcurrentStreamTest.exceededMaxStreamsPropagatesFailureCorrectly()less flaky.
DefaultRequestLog.isAvailable()Result:
Http2Exception: failed to consume a HEADERS frameerror.You tried to set the content preview twicewarning.RequestLogAccess.isAvailable()implementation.RequestLogBuilder.defer*Content*()andisDefer*Content*Set()have been deprecated in favor ofdefer(RequestLogProperty)andisDeferred(RequestLogProperty).