Skip to content

Add half_close state in Http2ClientSession #1704

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
masaori335 merged 1 commit into from
May 8, 2017
Merged

Add half_close state in Http2ClientSession #1704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions proxy/http2/Http2ClientSession.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ Http2ClientSession::Http2ClientSession()
sm_writer(nullptr),
upgrade_context(),
kill_me(false),
half_close(false),
recursion(0)
{
}
Expand Down Expand Up @@ -284,6 +285,15 @@ Http2ClientSession::reenable(VIO *vio)
this->client_vc->reenable(vio);
}

void
Http2ClientSession::set_half_close_flag(bool flag)
{
if (!half_close && flag) {
DebugHttp2Ssn("session half-close");
}
half_close = flag;
}

int
Http2ClientSession::main_event_handler(int event, void *edata)
{
Expand Down Expand Up @@ -496,6 +506,8 @@ Http2ClientSession::state_process_frame_read(int event, VIO *vio, bool inside_fr
SCOPED_MUTEX_LOCK(lock, this->connection_state.mutex, this_ethread());
if (!this->connection_state.is_state_closed()) {
this->connection_state.send_goaway_frame(this->connection_state.get_latest_stream_id_in(), err);
this->set_half_close_flag(true);
this->do_io_close();
}
}
return 0;
Expand Down
8 changes: 8 additions & 0 deletions proxy/http2/Http2ClientSession.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,13 @@ class Http2ClientSession : public ProxyClientSession
return retval;
}

void set_half_close_flag(bool flag) override;
bool
get_half_close_flag() const override
{
return half_close;
}

private:
Http2ClientSession(Http2ClientSession &); // noncopyable
Http2ClientSession &operator=(const Http2ClientSession &); // noncopyable
Expand Down Expand Up @@ -316,6 +323,7 @@ class Http2ClientSession : public ProxyClientSession
VIO *write_vio;
int dying_event;
bool kill_me;
bool half_close;
int recursion;
};

Expand Down
68 changes: 43 additions & 25 deletions proxy/http2/Http2ConnectionState.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,8 @@ Http2ConnectionState::main_event_handler(int event, void *edata)

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

@sidhuagarwal sidhuagarwal May 1, 2017

Choose a reason for hiding this comment

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

should this be ua_session's mutex?

Copy link
Contributor Author

@masaori335 masaori335 May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

@sidhuagarwal sidhuagarwal May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@masaori335 masaori335 May 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@masaori335 masaori335 May 2, 2017

Choose a reason for hiding this comment

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

@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?

Copy link

@sidhuagarwal sidhuagarwal May 2, 2017

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@masaori335 masaori335 May 2, 2017

Choose a reason for hiding this comment

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

@sidhuagarwal Got it. Thanks.


ink_assert(this->fini_received == false);
this->fini_received = true;
cleanup_streams();
Expand Down Expand Up @@ -898,17 +900,11 @@ Http2ConnectionState::main_event_handler(int event, void *edata)
error.msg);
}
this->send_goaway_frame(this->latest_streamid_in, error.code);
this->ua_session->set_half_close_flag(true);
Copy link
Member

@zizhong zizhong May 2, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@masaori335 masaori335 May 2, 2017

Choose a reason for hiding this comment

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

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

this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI);

// The streams will be cleaned up by the HTTP2_SESSION_EVENT_FINI event
// The Http2ClientSession will shutdown because connection_state.is_state_closed() will be true

// XXX We need to think a bit harder about how to coordinate the client
// session and the
// protocol connection. At this point, the protocol is shutting down,
// but there's no way
// to tell that to the client session. Perhaps this could be solved by
// implementing the
// half-closed state ...
SET_HANDLER(&Http2ConnectionState::state_closed);
} else if (error.cls == Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM) {
if (error.msg) {
Error("HTTP/2 stream error client_ip=%s session_id=%" PRId64 " %s", client_ip, ua_session->connection_id(), error.msg);
Expand Down Expand Up @@ -947,6 +943,13 @@ Http2ConnectionState::state_closed(int /* event */, void * /* edata */)
Http2Stream *
Http2ConnectionState::create_stream(Http2StreamId new_id, Http2Error &error)
{
// In half_close state, TS doesn't create new stream. Because GOAWAY frame is sent to client
if (ua_session && ua_session->get_half_close_flag()) {
error = Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM,
"refused to create new stream, because ua_session is in half_close state");
return nullptr;
}

bool client_streamid = http2_is_client_streamid(new_id);

// 5.1.1 The identifier of a newly established stream MUST be numerically
Expand Down Expand Up @@ -1057,6 +1060,8 @@ Http2ConnectionState::cleanup_streams()
ink_assert(stream_list.empty());

if (!is_state_closed()) {
SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread());

ua_session->get_netvc()->add_to_keep_alive_queue();
ua_session->get_netvc()->cancel_active_timeout();
}
Expand Down Expand Up @@ -1110,20 +1115,24 @@ Http2ConnectionState::release_stream(Http2Stream *stream)
stream_list.remove(stream);
}

// If the number of clients is 0 and ua_session is active, then mark the connection as inactive
if (total_client_streams_count == 0 && ua_session && ua_session->is_active()) {
ua_session->clear_session_active();
UnixNetVConnection *vc = static_cast<UnixNetVConnection *>(ua_session->get_netvc());
if (vc) {
vc->cancel_active_timeout();
vc->add_to_keep_alive_queue();
if (ua_session) {
SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread());

// If the number of clients is 0 and ua_session is active, then mark the connection as inactive
if (total_client_streams_count == 0 && ua_session->is_active()) {
ua_session->clear_session_active();
UnixNetVConnection *vc = static_cast<UnixNetVConnection *>(ua_session->get_netvc());
if (vc) {
vc->cancel_active_timeout();
vc->add_to_keep_alive_queue();
}
}
}

if (ua_session && fini_received && total_client_streams_count == 0) {
// We were shutting down, go ahead and terminate the session
ua_session->destroy();
ua_session = nullptr;
if (fini_received && total_client_streams_count == 0) {
// We were shutting down, go ahead and terminate the session
ua_session->destroy();
ua_session = nullptr;
}
}
}

Expand Down Expand Up @@ -1351,6 +1360,9 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream)
// Change stream state
if (!stream->change_state(HTTP2_FRAME_TYPE_HEADERS, flags)) {
this->send_goaway_frame(this->latest_streamid_in, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR);
this->ua_session->set_half_close_flag(true);
this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI);

h2_hdr.destroy();
ats_free(buf);
return;
Expand Down Expand Up @@ -1509,6 +1521,9 @@ Http2ConnectionState::send_rst_stream_frame(Http2StreamId id, Http2ErrorCode ec)
if (stream != nullptr) {
if (!stream->change_state(HTTP2_FRAME_TYPE_RST_STREAM, 0)) {
this->send_goaway_frame(this->latest_streamid_in, Http2ErrorCode::HTTP2_ERROR_PROTOCOL_ERROR);
this->ua_session->set_half_close_flag(true);
this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI);

return;
}
}
Expand Down Expand Up @@ -1541,7 +1556,10 @@ Http2ConnectionState::send_settings_frame(const Http2ConnectionSettings &new_set

// Write settings to send buffer
if (!http2_write_settings(param, iov)) {
send_goaway_frame(this->latest_streamid_in, Http2ErrorCode::HTTP2_ERROR_INTERNAL_ERROR);
this->send_goaway_frame(this->latest_streamid_in, Http2ErrorCode::HTTP2_ERROR_INTERNAL_ERROR);
this->ua_session->set_half_close_flag(true);
this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_FINI);

return;
}
iov.iov_base = reinterpret_cast<uint8_t *>(iov.iov_base) + HTTP2_SETTINGS_PARAMETER_LEN;
Expand Down Expand Up @@ -1576,10 +1594,12 @@ Http2ConnectionState::send_ping_frame(Http2StreamId id, uint8_t flag, const uint
this->ua_session->handleEvent(HTTP2_SESSION_EVENT_XMIT, &ping);
}

// As for gracefull shutdown, TS should process outstanding stream as long as possible.
// As for signal connection error, TS should close connection immediately.
void
Http2ConnectionState::send_goaway_frame(Http2StreamId id, Http2ErrorCode ec)
{
DebugHttp2Stream(ua_session, id, "Send GOAWAY frame");
DebugHttp2Con(ua_session, "Send GOAWAY frame, last_stream_id: %d", id);

if (ec != Http2ErrorCode::HTTP2_ERROR_NO_ERROR) {
HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_CONNECTION_ERRORS_COUNT, this_ethread());
Expand All @@ -1600,8 +1620,6 @@ Http2ConnectionState::send_goaway_frame(Http2StreamId id, Http2ErrorCode ec)
// 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, nullptr);
}

void
Expand Down