Signal VC_EVENT_READ_COMPLETE when ATS received END_STREAM flag

[masaori335]

One of the fixes for #6313. As @maskit pointed out on #6401, the VC_EVENT_READ_COMPLETE event was not sent when ATS received trailing header fields after c55001b. It uncovered an issue of HttpTunnel ( this should be fixed in another PR ).

Update:
Pure END_STREAM DATA frame case could also trigger the crash. Prior to this change, it was ignored like the trailing header with the END_STREAM flag.

TODO:

• Add AuTest to cover this trailer case.
  I used nghttp with --trailer option in local tests. But I'm wondering we can use curl to avoid increasing dependencies of AuTest.

[shinrich]

I think requiring nghttp on the ci box is quite reasonable. A curl supporting HTTP2 is already pulling in the nghttp2 libraries. I already have added an autest that requires nghttp (which is currently being skipped on the CI).

[shinrich]

Does the state machine even understand and set up VIO's for trailing headers yet? I'd argue that this change doesn't help much until the state machine deals with trailing headers. And should it send READ_COMPLETE? Or an EOS?

[zwoop]

[approve ci centos]

[vmamidi]

Why is VC_EVENT_READ_COMPLETE not sent?

[shinrich]

As noted on PR #6401 the crashes we are seeing in our environment are not due to trailing headers.

[masaori335]

@shinrich

I didn't know nghttp command is already used in AuTest. Let's install nghttp on the box.

Does the state machine even understand and set up VIO's for trailing headers yet? I'd argue that this change doesn't help much until the state machine deals with trailing headers.

This change doesn't add trailing headers support. Just fix the missing READ_COMPLETE event with the trailing header.

And should it send READ_COMPLETE? Or an EOS?

Good point. I'm wondering that the EOS flag on HTTP/2 frame should trigger VC_EVENT_EOS instead of VC_EVENT_READ_COMPLETE. I'll dig more.

[masaori335]

@vmamidi As Susan described on #6313 & #6401, after c55001b change, Http2Stream signal VC_EVENT_READ_COMPLETE only if END_STREAM flag is set on the HTTP/2 frame.
In general case, the last DATA frame has the END_STREAM flag, but in this trailing header case, the last HEADERS frame has the END_STREAM flag, but it was just ignored.

[masaori335]

As for VC_EVENT_EOS vs VC_EVENT_READ_COMPLETE, I think VC_EVENT_READ_COMPLETE is appropriate here. Because VC_EVENT_EOS is signaled from UnixNetVConnection/SSLNetVConnection in error cases.

[masaori335]

I found another case of missing VC_EVENT_READ_COMPLETE signal on receiving END_STREAM flag. In some cases, this also triggers the crash.

[maskit]

The comment is confusing because this if statement doesn't check the flag. Combine the if blocks or rephrase the comment.

[maskit]

Looks reasonable.

[zwoop]

Cherry-picked to v9.0.x branch.