This is an automated email from the ASF dual-hosted git repository.
bcall pushed a commit to branch 8.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/8.0.x by this push:
new 3387fa2 Bug fixes to h2 buffering
3387fa2 is described below
commit 3387fa27e47f5e862e40c714edfacb64d1a0e34f
Author: Masaori Koshiba <masa...@apache.org>
AuthorDate: Thu Apr 9 13:33:03 2020 -0700
Bug fixes to h2 buffering
---
doc/admin-guide/files/records.config.en.rst | 2 +-
mgmt/RecordsConfig.cc | 2 +-
proxy/http2/HTTP2.cc | 2 +-
proxy/http2/Http2ClientSession.cc | 12 ++++--
proxy/http2/Http2ClientSession.h | 2 +
proxy/http2/Http2ConnectionState.cc | 59 ++++++++++++++++++++---------
proxy/http2/Http2ConnectionState.h | 6 ++-
proxy/http2/Http2Stream.cc | 30 +++++++++++++++
proxy/http2/Http2Stream.h | 11 +++---
9 files changed, 95 insertions(+), 31 deletions(-)
diff --git a/doc/admin-guide/files/records.config.en.rst
b/doc/admin-guide/files/records.config.en.rst
index 3dc98a7..e51ec77 100644
--- a/doc/admin-guide/files/records.config.en.rst
+++ b/doc/admin-guide/files/records.config.en.rst
@@ -3450,7 +3450,7 @@ HTTP/2 Configuration
:ts:cv:`proxy.config.http2.min_concurrent_streams_in`.
To disable, set to zero (``0``).
-.. ts:cv:: CONFIG proxy.config.http2.initial_window_size_in INT 1048576
+.. ts:cv:: CONFIG proxy.config.http2.initial_window_size_in INT 65535
:reloadable:
The initial window size for inbound connections.
diff --git a/mgmt/RecordsConfig.cc b/mgmt/RecordsConfig.cc
index df38162..7bf85b9 100644
--- a/mgmt/RecordsConfig.cc
+++ b/mgmt/RecordsConfig.cc
@@ -1306,7 +1306,7 @@ static const RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.http2.max_active_streams_in", RECD_INT, "0",
RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
- {RECT_CONFIG, "proxy.config.http2.initial_window_size_in", RECD_INT,
"1048576", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
+ {RECT_CONFIG, "proxy.config.http2.initial_window_size_in", RECD_INT,
"65535", RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
{RECT_CONFIG, "proxy.config.http2.max_frame_size", RECD_INT, "16384",
RECU_DYNAMIC, RR_NULL, RECC_STR, "^[0-9]+$", RECA_NULL}
,
diff --git a/proxy/http2/HTTP2.cc b/proxy/http2/HTTP2.cc
index a74415b..b81e1bd 100644
--- a/proxy/http2/HTTP2.cc
+++ b/proxy/http2/HTTP2.cc
@@ -725,7 +725,7 @@ uint32_t Http2::min_concurrent_streams_in = 10;
uint32_t Http2::max_active_streams_in = 0;
bool Http2::throttling = false;
uint32_t Http2::stream_priority_enabled = 0;
-uint32_t Http2::initial_window_size = 1048576;
+uint32_t Http2::initial_window_size = 65535;
uint32_t Http2::max_frame_size = 16384;
uint32_t Http2::header_table_size = 4096;
uint32_t Http2::max_header_list_size = 4294967295;
diff --git a/proxy/http2/Http2ClientSession.cc
b/proxy/http2/Http2ClientSession.cc
index 8b68db3..1d72da7 100644
--- a/proxy/http2/Http2ClientSession.cc
+++ b/proxy/http2/Http2ClientSession.cc
@@ -360,11 +360,9 @@ Http2ClientSession::main_event_handler(int event, void
*edata)
break;
case VC_EVENT_WRITE_READY:
- retval = 0;
- break;
-
case VC_EVENT_WRITE_COMPLETE:
- // Seems as this is being closed already
+ this->connection_state.restart_streams();
+
retval = 0;
break;
@@ -627,3 +625,9 @@ Http2ClientSession::_should_do_something_else()
// Do something else every 128 incoming frames
return (this->_n_frame_read & 0x7F) == 0;
}
+
+int64_t
+Http2ClientSession::write_avail()
+{
+ return this->write_buffer->write_avail();
+}
diff --git a/proxy/http2/Http2ClientSession.h b/proxy/http2/Http2ClientSession.h
index e8b43a5..21ce079 100644
--- a/proxy/http2/Http2ClientSession.h
+++ b/proxy/http2/Http2ClientSession.h
@@ -316,6 +316,8 @@ public:
return write_buffer->max_read_avail();
}
+ int64_t write_avail();
+
// noncopyable
Http2ClientSession(Http2ClientSession &) = delete;
Http2ClientSession &operator=(const Http2ClientSession &) = delete;
diff --git a/proxy/http2/Http2ConnectionState.cc
b/proxy/http2/Http2ConnectionState.cc
index 5fc3a9e..3eaa558 100644
--- a/proxy/http2/Http2ConnectionState.cc
+++ b/proxy/http2/Http2ConnectionState.cc
@@ -151,6 +151,12 @@ rcv_data_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
cstate.decrement_server_rwnd(payload_length);
stream->decrement_server_rwnd(payload_length);
+ if (is_debug_tag_set("http2_con")) {
+ uint32_t rwnd =
cstate.server_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
+ Http2StreamDebug(cstate.ua_session, stream->get_id(), "Received DATA
frame: rwnd con=%zd/%" PRId32 " stream=%zd/%" PRId32,
+ cstate.server_rwnd(), rwnd, stream->server_rwnd(), rwnd);
+ }
+
const uint32_t unpadded_length = payload_length - pad_length;
// If we call write() multiple times, we must keep the same reader, so we can
// update its offset via consume. Otherwise, we will read the same data on
the
@@ -172,21 +178,6 @@ rcv_data_frame(Http2ConnectionState &cstate, const
Http2Frame &frame)
}
myreader->writer()->dealloc_reader(myreader);
- uint32_t initial_rwnd =
cstate.server_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
- uint32_t min_rwnd = std::min(initial_rwnd,
cstate.server_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
- // Connection level WINDOW UPDATE
- if (cstate.server_rwnd() <= min_rwnd) {
- Http2WindowSize diff_size = initial_rwnd - cstate.server_rwnd();
- cstate.increment_server_rwnd(diff_size);
- cstate.send_window_update_frame(0, diff_size);
- }
- // Stream level WINDOW UPDATE
- if (stream->server_rwnd() <= min_rwnd) {
- Http2WindowSize diff_size = initial_rwnd - stream->server_rwnd();
- stream->increment_server_rwnd(diff_size);
- cstate.send_window_update_frame(stream->get_id(), diff_size);
- }
-
return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE);
}
@@ -1217,6 +1208,35 @@ Http2ConnectionState::restart_streams()
}
void
+Http2ConnectionState::restart_receiving(Http2Stream *stream)
+{
+ uint32_t initial_rwnd =
this->server_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
+ uint32_t min_rwnd = std::min(initial_rwnd,
this->server_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
+
+ // Connection level WINDOW UPDATE
+ if (this->server_rwnd() < min_rwnd) {
+ Http2WindowSize diff_size = initial_rwnd - this->server_rwnd();
+ this->increment_server_rwnd(diff_size);
+ this->send_window_update_frame(0, diff_size);
+ }
+
+ // Stream level WINDOW UPDATE
+ if (stream == nullptr || stream->server_rwnd() >= min_rwnd) {
+ return;
+ }
+
+ // If read_vio is buffering data, do not fully update window
+ int64_t data_size = stream->read_vio_read_avail();
+ if (data_size >= initial_rwnd) {
+ return;
+ }
+
+ Http2WindowSize diff_size = initial_rwnd -
std::max(static_cast<int64_t>(stream->server_rwnd()), data_size);
+ stream->increment_server_rwnd(diff_size);
+ this->send_window_update_frame(stream->get_id(), diff_size);
+}
+
+void
Http2ConnectionState::cleanup_streams()
{
Http2Stream *s = stream_list.head;
@@ -1434,6 +1454,12 @@ Http2ConnectionState::send_a_data_frame(Http2Stream
*stream, size_t &payload_len
return Http2SendDataFrameResult::ERROR;
}
+ SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread());
+ if (this->ua_session->write_avail() == 0) {
+ Http2StreamDebug(this->ua_session, stream->get_id(), "Not write avail");
+ return Http2SendDataFrameResult::NOT_WRITE_AVAIL;
+ }
+
// Select appropriate payload length
if (read_available_size > 0) {
// We only need to check for window size when there is a payload
@@ -1477,7 +1503,6 @@ Http2ConnectionState::send_a_data_frame(Http2Stream
*stream, size_t &payload_len
current_reader->consume(payload_length);
// xmit event
- SCOPED_MUTEX_LOCK(lock, this->ua_session->mutex, this_ethread());
this->ua_session->handleEvent(HTTP2_SESSION_EVENT_XMIT, &data);
if (flags & HTTP2_FLAGS_DATA_END_STREAM) {
@@ -1865,7 +1890,7 @@ Http2ConnectionState::send_goaway_frame(Http2StreamId id,
Http2ErrorCode ec)
void
Http2ConnectionState::send_window_update_frame(Http2StreamId id, uint32_t size)
{
- Http2StreamDebug(ua_session, id, "Send WINDOW_UPDATE frame");
+ Http2StreamDebug(ua_session, id, "Send WINDOW_UPDATE frame: size=%" PRIu32,
size);
// Create WINDOW_UPDATE frame
Http2Frame window_update(HTTP2_FRAME_TYPE_WINDOW_UPDATE, id, 0x0);
diff --git a/proxy/http2/Http2ConnectionState.h
b/proxy/http2/Http2ConnectionState.h
index e6f4ff9..aab27cb 100644
--- a/proxy/http2/Http2ConnectionState.h
+++ b/proxy/http2/Http2ConnectionState.h
@@ -36,6 +36,7 @@ enum class Http2SendDataFrameResult {
NO_ERROR = 0,
NO_WINDOW,
NO_PAYLOAD,
+ NOT_WRITE_AVAIL,
ERROR,
DONE,
};
@@ -130,6 +131,8 @@ public:
void
init()
{
+ this->_server_rwnd = Http2::initial_window_size;
+
local_hpack_handle = new HpackHandle(HTTP2_HEADER_TABLE_SIZE);
remote_hpack_handle = new HpackHandle(HTTP2_HEADER_TABLE_SIZE);
dependency_tree = new DependencyTree(Http2::max_concurrent_streams_in);
@@ -173,6 +176,7 @@ public:
void release_stream(Http2Stream *stream);
void cleanup_streams();
+ void restart_receiving(Http2Stream *stream);
void update_initial_rwnd(Http2WindowSize new_size);
Http2StreamId
@@ -352,7 +356,7 @@ private:
// Connection level window size
ssize_t _client_rwnd = HTTP2_INITIAL_WINDOW_SIZE;
- ssize_t _server_rwnd = Http2::initial_window_size;
+ ssize_t _server_rwnd = 0;
std::vector<size_t> _recent_rwnd_increment = {SIZE_MAX, SIZE_MAX, SIZE_MAX,
SIZE_MAX, SIZE_MAX};
int _recent_rwnd_increment_index = 0;
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index d1c37f6..a4ef29e 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -519,6 +519,19 @@ Http2Stream::update_read_request(int64_t read_len, bool
call_update, bool check_
void
Http2Stream::restart_sending()
{
+ if (!this->response_header_done) {
+ return;
+ }
+
+ IOBufferReader *reader = this->response_get_data_reader();
+ if (reader && !reader->is_read_avail_more_than(0)) {
+ return;
+ }
+
+ if (this->write_vio.mutex && this->write_vio.ntodo() == 0) {
+ return;
+ }
+
this->send_response_body(true);
}
@@ -690,6 +703,12 @@ Http2Stream::reenable(VIO *vio)
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
update_write_request(vio->get_reader(), INT64_MAX, true);
} else if (vio->op == VIO::READ) {
+ Http2ClientSession *h2_proxy_ssn = static_cast<Http2ClientSession
*>(this->get_parent());
+ {
+ SCOPED_MUTEX_LOCK(lock, h2_proxy_ssn->connection_state.mutex,
this_ethread());
+ h2_proxy_ssn->connection_state.restart_receiving(this);
+ }
+
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
update_read_request(INT64_MAX, true);
}
@@ -945,3 +964,14 @@ Http2Stream::_switch_thread_if_not_on_right_thread(int
event, void *edata)
}
return true;
}
+
+int64_t
+Http2Stream::read_vio_read_avail()
+{
+ MIOBuffer *writer = this->read_vio.get_writer();
+ if (writer) {
+ return writer->max_read_avail();
+ }
+
+ return 0;
+}
diff --git a/proxy/http2/Http2Stream.h b/proxy/http2/Http2Stream.h
index 3be1057..f46404e 100644
--- a/proxy/http2/Http2Stream.h
+++ b/proxy/http2/Http2Stream.h
@@ -38,10 +38,7 @@ class Http2Stream : public ProxyClientTransaction
{
public:
typedef ProxyClientTransaction super; ///< Parent type.
- Http2Stream(Http2StreamId sid = 0, ssize_t initial_rwnd =
Http2::initial_window_size) : _id(sid), _client_rwnd(initial_rwnd)
- {
- SET_HANDLER(&Http2Stream::main_event_handler);
- }
+ Http2Stream() { SET_HANDLER(&Http2Stream::main_event_handler); }
void
init(Http2StreamId sid, ssize_t initial_rwnd)
@@ -50,6 +47,7 @@ public:
_start_time = Thread::get_hrtime();
_thread = this_ethread();
this->_client_rwnd = initial_rwnd;
+ this->_server_rwnd = Http2::initial_window_size;
HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_CURRENT_CLIENT_STREAM_COUNT,
_thread);
HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_TOTAL_CLIENT_STREAM_COUNT,
_thread);
sm_reader = request_reader = request_buffer.alloc_reader();
@@ -160,6 +158,7 @@ public:
ssize_t server_rwnd() const;
Http2ErrorCode increment_server_rwnd(size_t amount);
Http2ErrorCode decrement_server_rwnd(size_t amount);
+ int64_t read_vio_read_avail();
uint8_t *header_blocks = nullptr;
uint32_t header_blocks_length = 0; // total length of header blocks (not
include
@@ -275,8 +274,8 @@ private:
uint64_t data_length = 0;
uint64_t bytes_sent = 0;
- ssize_t _client_rwnd;
- ssize_t _server_rwnd = Http2::initial_window_size;
+ ssize_t _client_rwnd = 0;
+ ssize_t _server_rwnd = 0;
std::vector<size_t> _recent_rwnd_increment = {SIZE_MAX, SIZE_MAX, SIZE_MAX,
SIZE_MAX, SIZE_MAX};
int _recent_rwnd_increment_index = 0;
