Skip to content

Notify http2 error handler before closeStreamLocal on HEADERS write failure#8332

Merged
ejona86 merged 1 commit intonetty:4.1from
ejona86:trailers-encoding-error-order
Sep 28, 2018
Merged

Notify http2 error handler before closeStreamLocal on HEADERS write failure#8332
ejona86 merged 1 commit intonetty:4.1from
ejona86:trailers-encoding-error-order

Conversation

@ejona86
Copy link
Copy Markdown
Member

@ejona86 ejona86 commented Sep 27, 2018

Motivation:

When writing an HTTP/2 HEADERS with END_STREAM=1, the application expects
the stream to be closed afterward. However, the write can fail locally
due to HPACK encoder and similar. When that happens we need to make sure
to issue a RST_STREAM otherwise the stream can be closed locally but
orphaned remotely. The RST_STREAM is typically handled by
Http2ConnectionHandler.onStreamError, which will only send a RST_STREAM
if that stream still exists locally.

There are two possible flows for trailers, one handled immediately and
one going through the flow controller. Previously they behaved
differently, with the immedate code calling the error handler after
closing the stream. The immediate code also used a listener for calling
closeStreamLocal while the flow controlled code did so immediately after
the write.

The two code paths also differed in their VoidChannelPromise handling,
but both were broken. The immediate code path called unvoid() only if
END_STREAM=1, however it could always potentially add a listener via
notifyLifecycleManagerOnError(). And the flow controlled code path
unvoided incorrectly, changing the promise completion behavior. It also
passed the wrong promise to closeStreamLocal() in FlowControlledBase.

Modifications:

Move closeStreamLocal handling after calls to onError. This is the
primary change.

Now call closeStreamLocal immediately instead of when the future
completes. This is the more likely correct behavior as it matches that
of DATA frames.

Fix all the VoidChannelPromise handling.

Result:

Http2ConnectionHandler.onStreamError sees the same state as the remote
and issues a RST_STREAM, properly cleaning up the stream.


CC @normanmaurer, @Scottmitch

This issue impacts trailers and headers-only responses with malformed headers.
This was breaking a new gRPC test I wrote that tested MAX_HEADER_LIST_SIZE
behavior and caused the client RPC to time out because the server never sent a
response.

This does raise an oddity that we will closeStreamLocal() even if the END_STREAM=1
frame failed. That risks the local and remote states becoming out-of-sync and today
works because of Http2ConnectionHandler's behavior. However, given
Http2ConnectionHandler's current behavior it's not necessary at all to closeStreamLocal()
on failure; the rstStream will do that. Any change there seems like it should be a separate
discussion though.

@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

…ailure

Motivation:

When writing an HTTP/2 HEADERS with END_STREAM=1, the application expects
the stream to be closed afterward. However, the write can fail locally
due to HPACK encoder and similar. When that happens we need to make sure
to issue a RST_STREAM otherwise the stream can be closed locally but
orphaned remotely. The RST_STREAM is typically handled by
Http2ConnectionHandler.onStreamError, which will only send a RST_STREAM
if that stream still exists locally.

There are two possible flows for trailers, one handled immediately and
one going through the flow controller. Previously they behaved
differently, with the immedate code calling the error handler after
closing the stream. The immediate code also used a listener for calling
closeStreamLocal while the flow controlled code did so immediately after
the write.

The two code paths also differed in their VoidChannelPromise handling,
but both were broken. The immediate code path called unvoid() only if
END_STREAM=1, however it could always potentially add a listener via
notifyLifecycleManagerOnError(). And the flow controlled code path
unvoided incorrectly, changing the promise completion behavior. It also
passed the wrong promise to closeStreamLocal() in FlowControlledBase.

Modifications:

Move closeStreamLocal handling after calls to onError. This is the
primary change.

Now call closeStreamLocal immediately instead of when the future
completes. This is the more likely correct behavior as it matches that
of DATA frames.

Fix all the VoidChannelPromise handling.

Result:

Http2ConnectionHandler.onStreamError sees the same state as the remote
and issues a RST_STREAM, properly cleaning up the stream.
@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot add to whitelist.

@ejona86 ejona86 force-pushed the trailers-encoding-error-order branch from d65453e to 3c0bf0a Compare September 27, 2018 19:08
@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@ejona86
Copy link
Copy Markdown
Member Author

ejona86 commented Sep 27, 2018

CC @carl-mastrangelo

@normanmaurer
Copy link
Copy Markdown
Member

@bryce-anderson @mosesn would love to have you review as well .

Copy link
Copy Markdown
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

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

lgtm

@normanmaurer
Copy link
Copy Markdown
Member

@trustin please check as well

Copy link
Copy Markdown
Member

@carl-mastrangelo carl-mastrangelo left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants