You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2024/02/23 21:34:13 UTC
[PR] NOT_WRITE_AVAIL: schedule event to resend data frames [trafficserver]
bneradt opened a new pull request, #11094:
URL: https://github.com/apache/trafficserver/pull/11094
When sending a DATA frames, we check that we are not exceeding our write
buffer's high water mark. If so, we don't write the frame.
Unfortunately, before this patch, no event was scheduled to come back
and try to resend any unwritten frames. This led to dropped frames at
times. This patch adds a scheduled event to later re-try sending the
unsent frames.
This patch also updates the grpc.test.py to send 100 messages instead of
1 which, at least on my local system, reproduced this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NOT_WRITE_AVAIL: schedule event to resend data frames [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #11094:
URL: https://github.com/apache/trafficserver/pull/11094#issuecomment-1988651886
Cherry-picked to v10.0.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NOT_WRITE_AVAIL: schedule event to resend data frames [trafficserver]
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #11094:
URL: https://github.com/apache/trafficserver/pull/11094#discussion_r1501199470
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -2014,24 +2024,39 @@ Http2ConnectionState::update_initial_local_rwnd(Http2WindowSize new_size)
}
void
-Http2ConnectionState::schedule_stream(Http2Stream *stream)
+Http2ConnectionState::schedule_stream_to_send_priority_frames(Http2Stream *stream)
{
- Http2StreamDebug(session, stream->get_id(), "Scheduled");
+ Http2StreamDebug(session, stream->get_id(), "Scheduling sending priority frames");
Http2DependencyTree::Node *node = stream->priority_node;
ink_release_assert(node != nullptr);
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
dependency_tree->activate(node);
- if (!_scheduled) {
- _scheduled = true;
+ if (!_priority_scheduled) {
+ _priority_scheduled = true;
SET_HANDLER(&Http2ConnectionState::main_event_handler);
this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_XMIT);
}
}
+void
+Http2ConnectionState::schedule_stream_to_send_data_frames(Http2Stream *stream)
+{
+ Http2StreamDebug(session, stream->get_id(), "Scheduling sending data frames");
+
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
+ if (!_data_scheduled) {
+ _data_scheduled = true;
+
+ SET_HANDLER(&Http2ConnectionState::main_event_handler);
+ this_ethread()->schedule_in((Continuation *)this, HRTIME_MSECOND, HTTP2_SESSION_EVENT_DATA);
Review Comment:
I don't think it matter that much. But your reasoning to leave a bit of time to drain seems sound. Since we failed the first time, this isn't going to be the fastest path anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NOT_WRITE_AVAIL: schedule event to resend data frames [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #11094:
URL: https://github.com/apache/trafficserver/pull/11094#discussion_r1501192938
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -2014,24 +2024,39 @@ Http2ConnectionState::update_initial_local_rwnd(Http2WindowSize new_size)
}
void
-Http2ConnectionState::schedule_stream(Http2Stream *stream)
+Http2ConnectionState::schedule_stream_to_send_priority_frames(Http2Stream *stream)
{
- Http2StreamDebug(session, stream->get_id(), "Scheduled");
+ Http2StreamDebug(session, stream->get_id(), "Scheduling sending priority frames");
Http2DependencyTree::Node *node = stream->priority_node;
ink_release_assert(node != nullptr);
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
dependency_tree->activate(node);
- if (!_scheduled) {
- _scheduled = true;
+ if (!_priority_scheduled) {
+ _priority_scheduled = true;
SET_HANDLER(&Http2ConnectionState::main_event_handler);
this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_XMIT);
}
}
+void
+Http2ConnectionState::schedule_stream_to_send_data_frames(Http2Stream *stream)
+{
+ Http2StreamDebug(session, stream->get_id(), "Scheduling sending data frames");
+
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
+ if (!_data_scheduled) {
+ _data_scheduled = true;
+
+ SET_HANDLER(&Http2ConnectionState::main_event_handler);
+ this_ethread()->schedule_in((Continuation *)this, HRTIME_MSECOND, HTTP2_SESSION_EVENT_DATA);
Review Comment:
I chose to schedule this for 1ms in the future. Does this sound reasonable to people? I figure time should be allotted for the buffer to drain. But maybe schedule_imm is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NOT_WRITE_AVAIL: schedule event to resend data frames [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #11094:
URL: https://github.com/apache/trafficserver/pull/11094#discussion_r1501214325
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -2014,24 +2024,39 @@ Http2ConnectionState::update_initial_local_rwnd(Http2WindowSize new_size)
}
void
-Http2ConnectionState::schedule_stream(Http2Stream *stream)
+Http2ConnectionState::schedule_stream_to_send_priority_frames(Http2Stream *stream)
{
- Http2StreamDebug(session, stream->get_id(), "Scheduled");
+ Http2StreamDebug(session, stream->get_id(), "Scheduling sending priority frames");
Http2DependencyTree::Node *node = stream->priority_node;
ink_release_assert(node != nullptr);
SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
dependency_tree->activate(node);
- if (!_scheduled) {
- _scheduled = true;
+ if (!_priority_scheduled) {
+ _priority_scheduled = true;
SET_HANDLER(&Http2ConnectionState::main_event_handler);
this_ethread()->schedule_imm_local((Continuation *)this, HTTP2_SESSION_EVENT_XMIT);
}
}
+void
+Http2ConnectionState::schedule_stream_to_send_data_frames(Http2Stream *stream)
+{
+ Http2StreamDebug(session, stream->get_id(), "Scheduling sending data frames");
+
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+
+ if (!_data_scheduled) {
+ _data_scheduled = true;
+
+ SET_HANDLER(&Http2ConnectionState::main_event_handler);
+ this_ethread()->schedule_in((Continuation *)this, HRTIME_MSECOND, HTTP2_SESSION_EVENT_DATA);
Review Comment:
Good point - we're already delayed behind the buffer, so sleeping the ms is probably reasonable. Thanks @shinrich .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NOT_WRITE_AVAIL: schedule event to resend data frames [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #11094:
URL: https://github.com/apache/trafficserver/pull/11094
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org