Add half_close state in Http2ClientSession #1704

masaori335 · 2017-04-19T00:11:42Z

This could be fix of #1673

Approach

Add half_close flag in Http2ClientSession like Http1ClientSession.
Set half_close true at end of send_goaway_frame()
Wait VC_EVENT_WRITE_COMPLETE event (probably GOAWAY frame is written)
Close, destroy and free client_vc and ua_session

Concerns

My concern is that the VC_EVENT_WRITE_COMPLETE event is exactly the event of sending GOAWAY frame or not. When some frame is already scheduled to be written (but not written) before send_goaway_frame() is called, same issue will be happen?
Is there any way to make sure the event is the event of written of GOAWAY frame?

Tests

h2spec (v2.1.0) http2/6.9.1/2 is passed.

$ ./h2spec http2/6.9.1/2 -p 4443 -k -t -v
Hypertext Transfer Protocol Version 2 (HTTP/2)
  6. Frame Definitions
    6.9. WINDOW_UPDATE
      6.9.1. The Flow-Control Window
             [send] SETTINGS Frame (length:6, flags:0x00, stream_id:0)
             [recv] SETTINGS Frame (length:12, flags:0x00, stream_id:0)
             [send] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
             [recv] WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)
             [recv] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
             [send] WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)
             [send] WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)
             [recv] GOAWAY Frame (length:8, flags:0x00, stream_id:0)
        ✔ 2: Sends multiple WINDOW_UPDATE frames increasing the flow control window to above 2^31-1

Finished in 0.0209 seconds
1 tests, 1 passed, 0 skipped, 0 failed

masaori335 · 2017-04-19T00:19:14Z

@zizhong Would you review this?

zizhong · 2017-04-19T00:26:23Z

@masaori335 yes, the race condition you mentioned is definitely an issue. We might have a better way to do this. 🤔

zwoop · 2017-04-19T04:21:12Z

Is this a 7.1.x candidate?

maskit · 2017-04-19T15:45:02Z

How does a session get closed state?
We don't need to call SET_HANDLER anymore?
What would be the difference between closed and half-closed?

It seems like just changing the timing of closing process a bit later to me.

How about setting session inactive timeout short after scheduling of sending a GOAWAY frame? If you sent a GOAWAY frame, the session would be inactive eventually. We could assume the timeout as a sign of write completion of the GOAWAY frame. If you want to close a session asap, you can send RST_STREAM frames to all streams on the session to make them inactive immediately. It would be better to use active timeout too just in case.

sidhuagarwal · 2017-04-19T22:10:41Z

proxy/http2/Http2ClientSession.cc

    break;

  case VC_EVENT_WRITE_COMPLETE:
+    if (this->get_half_close_flag()) {


When the GOAWAY frame is sent, the sender should complete the processing of outstanding streams which are less than the stream identifier sent in the goaway frame. It seems we are not doing that here.

According to the specification, there's no guarantee of the completion. The stream identifier in the GOAWAY frame says that streams which have higher stream id than the one in GOAWAY frame are not processed at all.

https://tools.ietf.org/html/rfc7540#section-6.8

The last stream identifier in the GOAWAY frame contains the highest- numbered stream identifier for which the sender of the GOAWAY frame might have taken some action on or might yet take action on. All streams up to and including the identified stream might have been processed in some way. The last stream identifier can be set to 0 if no streams were processed.

Good point. As far as I understand, there're 2 scenarios for sending GOAWAY frame, Graceful Shutdown and Connection Error Handling. I'm sure that it is better to proceed outstanding streams as long as possible for Graceful Shutdown (maybe until active or inactive timeout). Should we do same thing for Connection Error Handling?

I agree that there is no guarantee, but we should try to process the outstanding streams as long as possible. Two questions.

Once a GOAWAY frame is sent, shouldn't we stop accepting new streams?

should we close the session only when there are no outstanding streams?

Yes, we should try that. The point here is how much the "as long as possible" is.

> 1.
We should stop accepting new steams. The reason is we could not increase the last stream id.

Endpoints MUST NOT increase the value they send in the last stream identifier, since the peers might already have retried unprocessed requests on another connection

So we should ignore the frame. Probably it's better to send RST_STREAM frame.

> 2.
We should also close the session by active timeout or inactive timeout. Even if there're outstanding streams, we should not leave the connection for long time after sending GOAWAY frame. Especially when TS is handling Connection Error.

@sidhuagarwal

Yes, we should stop accepting new streams. I think we can respond with RST_STREAM frame for HEADERS frames newly arrived.

Yes and no. Not only. Basically we should keep processing streams already open as long as possible, as you commented. However, if one of streams were transferring gigabytes file, it may take long time. In this case we want to close the stream even if it's not completed. This is the reason I mentioned active timeout.

Oops, masaori's comment wasn't showed in my browser. :)

Please be careful when we ignore the new incoming frames.

However, any frames that alter connection
state cannot be completely ignored. For instance, HEADERS,
PUSH_PROMISE, and CONTINUATION frames MUST be minimally processed to
ensure the state maintained for header compression is consistent (see
Section 4.3); similarly, DATA frames MUST be counted toward the
connection flow-control window. Failure to process these frames can
cause flow control or header compression state to become
unsynchronized.

One tiny change needs to be made related to the graceful shutdown. When sending the shutdown notice (stream_id == INT_MAX, error==NO_ERROR), we can't schedule session close. We can only schedule it if stream_id < INT_MAX.

zizhong · 2017-04-25T21:38:31Z

@masaori335 Where are we about this PR? Ping you because HTTP2 drain feature needs this as a prerequisite.

masaori335 · 2017-04-25T23:45:25Z

@zizhong Still work in progress. I tried to use inactivity_timeout and found another bug (#1716). Now, it's fixed, so I'm going to change this PR to use inactivity_timeout.

zizhong · 2017-04-25T23:53:31Z

@masaori335 Nice work! Thanks for the update.

atsci · 2017-04-26T03:10:33Z

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/310/

atsci · 2017-04-26T03:11:15Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/327/

atsci · 2017-04-26T03:11:15Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/314/

atsci · 2017-04-26T03:13:36Z

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/2008/

atsci · 2017-04-26T03:14:14Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/439/

atsci · 2017-04-26T03:14:28Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1901/

atsci · 2017-05-01T00:36:14Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/346/

atsci · 2017-05-01T00:43:39Z

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/2028/

atsci · 2017-05-01T00:44:07Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/459/

atsci · 2017-05-01T00:45:02Z

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/330/

atsci · 2017-05-01T00:49:44Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1921/

atsci · 2017-05-01T01:01:09Z

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/592/

maskit

Looks good to me.

sidhuagarwal · 2017-05-01T04:22:16Z

proxy/http2/Http2ConnectionState.cc


  // Finalize HTTP/2 Connection
  case HTTP2_SESSION_EVENT_FINI: {
+    SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());


should this be ua_session's mutex?

We need this because this protects data of Http2ConnectionState.
OTOH, it's worth to consider that adding ua_session's mutex lock. Because cleanup_streams() and release_stream() change data of ua_session.

We saw some crashes if the ua stream mutex is not held.

@sidhuagarwal Could you share how to reproduce? Run this patch with #1710?

@sidhuagarwal Added SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread()); in cleanup_streams() and release_stream(). Could you verify the crash doesn't happen?

@masaori335 we saw some crashes under high load when we were sending HTTP2_SESSION_EVENT_FINI without holding the ua_session's mutex. I don't have a reproduction case.

@sidhuagarwal Got it. Thanks.

Http2ClientSession is set half_close state after GOAWAY frame is sent to client. In half_close state, TS doesn't create new HTTP/2 stream.

atsci · 2017-05-02T00:51:20Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/352/

atsci · 2017-05-02T00:51:21Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/339/

atsci · 2017-05-02T00:54:06Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/465/

atsci · 2017-05-02T00:55:03Z

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/336/

atsci · 2017-05-02T00:58:42Z

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/2034/

atsci · 2017-05-02T00:59:25Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1927/

atsci · 2017-05-02T01:11:38Z

clang-analyzer build failed! https://ci.trafficserver.apache.org/job/clang-analyzer-github/598/

zizhong · 2017-05-02T05:24:41Z

proxy/http2/Http2ConnectionState.cc

                error.msg);
        }
        this->send_goaway_frame(this->latest_streamid_in, error.code);
+        this->ua_session->set_half_close_flag(true);


set_half_close_flag is changing the state of ua_session too. Should we hold a mutex for it too?

I think ua_session's mutex is already held. Because here is always on call stack from ua_session.

zwoop · 2017-05-08T14:55:57Z

Nice work. Two questions:

Did we want this for 7.1.0?
I think there's a new coverity issue reported after this PR. Please fix, and if we want this PR for 7.1.0, make sure to mark the fix for 7.1.0 as well.

masaori335 · 2017-05-09T00:08:13Z

@zwoop It's nice to have. But I don't have strong reasons to push this 7.1.0.

zwoop · 2017-05-11T14:55:05Z

Cherry picked to 7.1.0 per @maskit's request. :)

masaori335 added the HTTP/2 label Apr 19, 2017

masaori335 added this to the 7.2.0 milestone Apr 19, 2017

masaori335 requested review from maskit and shinrich April 19, 2017 00:13

masaori335 force-pushed the h2_half_close branch from 377f7bd to 37208e6 Compare April 19, 2017 00:21

masaori335 self-assigned this Apr 19, 2017

masaori335 added the WIP label Apr 19, 2017

masaori335 force-pushed the h2_half_close branch from 37208e6 to 792bbb5 Compare April 19, 2017 00:48

sidhuagarwal reviewed Apr 19, 2017

View reviewed changes

zizhong mentioned this pull request Apr 19, 2017

HTTP2 drain #1710

Merged

zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017

masaori335 force-pushed the h2_half_close branch 2 times, most recently from 88c98a6 to d0531ad Compare April 26, 2017 03:09

masaori335 removed the WIP label Apr 26, 2017

maskit approved these changes May 1, 2017

View reviewed changes

sidhuagarwal reviewed May 1, 2017

View reviewed changes

Add half_close state in Http2ClientSession

09c4610

Http2ClientSession is set half_close state after GOAWAY frame is sent to client. In half_close state, TS doesn't create new HTTP/2 stream.

masaori335 force-pushed the h2_half_close branch from 7342e39 to 09c4610 Compare May 2, 2017 00:45

sidhuagarwal approved these changes May 2, 2017

View reviewed changes

zizhong reviewed May 2, 2017

View reviewed changes

zizhong approved these changes May 2, 2017

View reviewed changes

zizhong mentioned this pull request May 2, 2017

h2spec: GOAWAY is needed for receiving frames after a stream closed. #1753

Closed

masaori335 merged commit daea3d7 into apache:master May 8, 2017

masaori335 mentioned this pull request May 8, 2017

Client never gets a GOAWAY frame #1673

Closed

masaori335 mentioned this pull request May 8, 2017

Coverity 1374845: Null pointer dereferences #1795

Merged

masaori335 added the Backport Marked for backport for an LTS patch release label May 11, 2017

masaori335 modified the milestones: 7.1.0, 8.0.0 May 11, 2017

Add half_close state in Http2ClientSession #1704

Add half_close state in Http2ClientSession #1704

Uh oh!

Conversation

masaori335 commented Apr 19, 2017

Approach

Concerns

Tests

Uh oh!

masaori335 commented Apr 19, 2017

Uh oh!

zizhong commented Apr 19, 2017

Uh oh!

zwoop commented Apr 19, 2017

Uh oh!

maskit commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zizhong Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zizhong commented Apr 25, 2017

Uh oh!

masaori335 commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zizhong commented Apr 25, 2017

Uh oh!

atsci commented Apr 26, 2017

Uh oh!

atsci commented Apr 26, 2017

Uh oh!

atsci commented Apr 26, 2017

Uh oh!

atsci commented Apr 26, 2017

Uh oh!

atsci commented Apr 26, 2017

Uh oh!

atsci commented Apr 26, 2017

Uh oh!

atsci commented May 1, 2017

Uh oh!

atsci commented May 1, 2017

Uh oh!

atsci commented May 1, 2017

Uh oh!

atsci commented May 1, 2017

Uh oh!

atsci commented May 1, 2017

Uh oh!

atsci commented May 1, 2017

Uh oh!

maskit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masaori335 May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidhuagarwal May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

zizhong Apr 20, 2017 •

edited

Loading

masaori335 commented Apr 25, 2017 •

edited

Loading

masaori335 May 1, 2017 •

edited

Loading

sidhuagarwal May 1, 2017 •

edited

Loading