quiche: add support of delayed close to QUIC session#9163
quiche: add support of delayed close to QUIC session#9163alyssawilk merged 17 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
This reverts commit 5642cf8. Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/assign @alyssawilk |
| delayed_close_timer_->enableTimer(delayed_close_timeout_); | ||
| } else { | ||
| closeConnectionImmediately(); | ||
| } |
There was a problem hiding this comment.
If there is data to write, don't we update the delay close timer to now + delay close?
There was a problem hiding this comment.
The timer is set when close() is called. I don't think we postpone it whenever we get a chance to write, do we?
There was a problem hiding this comment.
The docs say it's timeout interval after the last write, e.g.
// The socket will be closed immediately after the buffer is flushed or if a period of
// inactivity after the last write event greater than or equal to delayed_close_timeout_ has
// elapsed.
CloseAfterFlush,
and I believe the TCP stack re-arms the timer after each write
https://github.com/envoyproxy/envoy/blob/master/source/common/network/connection_impl.cc#L588
There was a problem hiding this comment.
Woops, I totally missed this. I changed OnCanWrite() to re-arm timer for that case now.
| } else { | ||
| delayed_close_state_ = DelayedCloseState::CloseAfterFlush; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I think we could remove this else, and just have else if (hasDataToWrite()) {} else { // type == NoFlush
| initializeDelayedCloseTimer(); | ||
| } | ||
| // Update delay close state according to current call. | ||
| if (delayed_close_timeout_configured && |
There was a problem hiding this comment.
maybe have one if for delayed_close_timeout_configured and have the above code and this code in it?
There was a problem hiding this comment.
Done. But due to the intertwine of ConnectionCloseType and the presence of delayed_close_time in config, I feel it doesn't simplify the logic much...
| quic_connection_->OnCanWrite(); | ||
| closeConnectionImmediately(); | ||
| } else { | ||
| // Quic connection doesn't have unsent data. It's upto caller and |
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
I sync'ed with #8496 and added client side implementation and integration test. PTAL |
Signed-off-by: Dan Zhang <danzh@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Looks much better - just a few more nits!
| "with the configured filter chain."); | ||
| connection_stats_ = std::make_unique<ConnectionStats>(stats); | ||
| } | ||
|
|
There was a problem hiding this comment.
These are format fixes for previous change
|
|
||
| void EnvoyQuicServerSession::OnCanWrite() { | ||
| quic::QuicServerSessionBase::OnCanWrite(); | ||
| // Do no update delay close state according to connection level packet egress because that is |
There was a problem hiding this comment.
Do no -> do not
Also yay for detailed commenting :-)
| write_buffer_watermark_simulation_.checkLowWatermark(bytes_to_send_); | ||
| } | ||
|
|
||
| void QuicFilterManagerConnectionImpl::maybeHandleDelayedClose() { |
There was a problem hiding this comment.
I think this could use better naming. By name I assumed this would be handling the close(... delayClose) call not applying the delay close policy after a write.
The comments in the .h file are great but if we can make the name more clear that'd be even better.
There was a problem hiding this comment.
rename to maybeApplyDelayClosePolicy()
Signed-off-by: Dan Zhang <danzh@google.com>
|
Ping? @alyssawilk |
Implement a TODO in QuicFilterManagerConnectionImpl::close() to delay close a quic connection. The behavior mimics ConnectionImpl::close(): If there is buffered data at quic stream or connection layer, wait for QUIC to drain all the data with a timeout if delayed close timeout is configured. After data is drained, close the connection if FlushWrite. If close() takes in FlushWriteAndWait and config has delayed close timeout, delay closing the connection even when there is no pending data or data has been drained. Risk Level: low, no in production Testing: added new unit tests Part of envoyproxy#2557 Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>
3 participants
Implement a TODO in QuicFilterManagerConnectionImpl::close() to delay close a quic connection.
The behavior mimics ConnectionImpl::close():
If there is buffered data at quic stream or connection layer, wait for QUIC to drain all the data with a timeout if delayed close timeout is configured. After data is drained, close the connection if FlushWrite.
If close() takes in FlushWriteAndWait and config has delayed close timeout, delay closing the connection even when there is no pending data or data has been drained.
Risk Level: low, no in production
Testing: added new unit tests
Part of #2557