Client never gets a GOAWAY frame

[zizhong]

currently, at the end of Http2ConnectionState::send_goaway_frame()

  http2_write_goaway(goaway, frame.write());
  frame.finalize(HTTP2_GOAWAY_LEN);

  // xmit event
  SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread());
  this->ua_session->handleEvent(HTTP2_SESSION_EVENT_XMIT, &frame);

  handleEvent(HTTP2_SESSION_EVENT_FINI, NULL);

it calls handleEvent(HTTP2_SESSION_EVENT_FINI, NULL), which calls cleanup_streams() and release_stream(), then ua_session->destroy(), and then closes the client_vc. so GOAWAY frame cannot be send.

From the code and local test result, the client can never get a GOAWAY frame.

[masaori335]

Looks like same issue of https://issues.apache.org/jira/browse/TS-3947.
Would you give us how to reproduce this?

[sidhuagarwal]

should this be scheduled on a thread and the we can call handleEvent(HTTP2_SESSION_EVENT_FINI, NULL), this way we give the buffer some time to clear and frame to be flushed to the client?

[zizhong]

@sidhuagarwal Inspiring. Let me try to get a fix from this idea.

[zizhong]

@masaori335 thanks for the fix. Let me test it on my local.

[zizhong]

I verified the fix. It works great. Thanks! @masaori335 @sidhuagarwal

Let me share with you the way to reproduce this issue. I have a patch zizhong@ba9703a that can drain http2 connections when trafficserver shuts down. The idea is to send GOAWAY frame to the client when stopping ATS.

You can easily reproduce the issue with the above patch.

1. set up ATS with an origin server responding data with a 20s delay.
2. send an h2 request
3. before the response returns, stop ATS

And with your fix, the client can finally receive the GOAWAY!

t=665949 [st=25094]    HTTP2_SESSION_GOAWAY
                       --> active_streams = 0
                       --> debug_data = "[0 bytes were stripped]"
                       --> last_accepted_stream_id = 1
                       --> status = 0
                       --> unclaimed_streams = 0
t=665949 [st=25094]    HTTP2_SESSION_CLOSE
                       --> description = "Finished going away"
                       --> net_error = 0 (?)

I feel ATS needs the http2 draining feature and my patch can be improved in a lot of ways. If you have any idea, I'm all ears.

[masaori335]

@zizhong Yes, http2 draining feature ( TS-3864 ) is desired but not implemented feature. Glad to see you have a branch for it:)
The half-closed state will be a piece of it. In that state, TS can ignore frames to open additional streams. One thing we should take care is TS should process streams which are already opened, as long as it possible.

[zwoop]

Isn't there a PR for this? If so, remember to close this when you merge the PR.