Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
1 change: 1 addition & 0 deletions source/common/network/connection_impl_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ void ConnectionImplBase::setConnectionStats(const ConnectionStats& stats) {
"with the configured filter chain.");
connection_stats_ = std::make_unique<ConnectionStats>(stats);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

revert?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are format fixes for previous change

void ConnectionImplBase::setDelayedCloseTimeout(std::chrono::milliseconds timeout) {
// Validate that this is only called prior to issuing a close() or closeSocket().
ASSERT(delayed_close_timer_ == nullptr && state() == State::Open);
Expand Down
1 change: 0 additions & 1 deletion source/common/network/connection_impl_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ class ConnectionImplBase : public FilterManagerConnection,
protected Logger::Loggable<Logger::Id::connection> {
public:
ConnectionImplBase(Event::Dispatcher& dispatcher, uint64_t id);
~ConnectionImplBase() override {}

// Network::Connection
void addConnectionCallbacks(ConnectionCallbacks& cb) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,26 @@ void EnvoyQuicServerSession::SendGoAway(quic::QuicErrorCode error_code, const st
}
}

void EnvoyQuicServerSession::OnCanWrite() {
quic::QuicServerSessionBase::OnCanWrite();
if (inDelayedClose() && !HasDataToWrite()) {
if (delayed_close_state_ == DelayedCloseState::CloseAfterFlushAndWait) {
ASSERT(delayed_close_timer_ != nullptr);
delayed_close_timer_->enableTimer(delayed_close_timeout_);
} else {
closeConnectionImmediately();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If there is data to write, don't we update the delay close timer to now + delay close?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The timer is set when close() is called. I don't think we postpone it whenever we get a chance to write, do we?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

cc @AndresGuedez

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Woops, I totally missed this. I changed OnCanWrite() to re-arm timer for that case now.

}
}

void EnvoyQuicServerSession::OnCryptoHandshakeEvent(CryptoHandshakeEvent event) {
quic::QuicServerSessionBase::OnCryptoHandshakeEvent(event);
if (event == HANDSHAKE_CONFIRMED) {
raiseConnectionEvent(Network::ConnectionEvent::Connected);
}
}

bool EnvoyQuicServerSession::hasDataToWrite() { return HasDataToWrite(); }

} // namespace Quic
} // namespace Envoy
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase,
quic::ConnectionCloseSource source) override;
void Initialize() override;
void SendGoAway(quic::QuicErrorCode error_code, const std::string& reason) override;
void OnCanWrite() override;
// quic::QuicSpdySession
void OnCryptoHandshakeEvent(CryptoHandshakeEvent event) override;

Expand All @@ -68,6 +69,9 @@ class EnvoyQuicServerSession : public quic::QuicServerSessionBase,
quic::QuicSpdyStream* CreateOutgoingBidirectionalStream() override;
quic::QuicSpdyStream* CreateOutgoingUnidirectionalStream() override;

// QuicFilterManagerConnectionImpl
bool hasDataToWrite() override;

private:
void setUpRequestDecoder(EnvoyQuicStream& stream);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,20 +50,43 @@ bool QuicFilterManagerConnectionImpl::aboveHighWatermark() const {
}

void QuicFilterManagerConnectionImpl::close(Network::ConnectionCloseType type) {
if (type != Network::ConnectionCloseType::NoFlush) {
// TODO(danzh): Implement FlushWrite and FlushWriteAndDelay mode.
}
if (quic_connection_ == nullptr) {
// Already detached from quic connection.
return;
}
closeConnectionImmediately();
}

void QuicFilterManagerConnectionImpl::setDelayedCloseTimeout(std::chrono::milliseconds timeout) {
if (timeout != std::chrono::milliseconds::zero()) {
// TODO(danzh) support delayed close of connection.
NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
const bool delayed_close_timeout_configured = delayed_close_timeout_.count() > 0;
if (hasDataToWrite() && type != Network::ConnectionCloseType::NoFlush) {
// QUIC connection has unsent data and caller wants to flush them. Wait for flushing or timeout.
if (!inDelayedClose() && delayed_close_timeout_configured) {
// Only set alarm if not in delay close mode yet.
initializeDelayedCloseTimer();
}
// Update delay close state according to current call.
if (delayed_close_timeout_configured &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe have one if for delayed_close_timeout_configured and have the above code and this code in it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

type == Network::ConnectionCloseType::FlushWriteAndDelay) {
delayed_close_state_ = DelayedCloseState::CloseAfterFlushAndWait;
} else {
delayed_close_state_ = DelayedCloseState::CloseAfterFlush;
}
} else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we could remove this else, and just have else if (hasDataToWrite()) {} else { // type == NoFlush

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point! Done

if (hasDataToWrite()) {
// Quic connection has unsent data but caller wants to close right away.
ASSERT(type == Network::ConnectionCloseType::NoFlush);
quic_connection_->OnCanWrite();
closeConnectionImmediately();
} else {
// Quic connection doesn't have unsent data. It's upto caller and

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

upto -> up to the

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

// configuration whether to wait or not before closing.
if (delayed_close_timeout_configured &&
type == Network::ConnectionCloseType::FlushWriteAndDelay) {
if (!inDelayedClose()) {
initializeDelayedCloseTimer();
}
delayed_close_state_ = DelayedCloseState::CloseAfterFlushAndWait;
} else {
closeConnectionImmediately();
}
}
}
}

Expand Down Expand Up @@ -115,6 +138,9 @@ void QuicFilterManagerConnectionImpl::onConnectionCloseEvent(
}

void QuicFilterManagerConnectionImpl::closeConnectionImmediately() {
if (quic_connection_ == nullptr) {
return;
}
quic_connection_->CloseConnection(quic::QUIC_NO_ERROR, "Closed by application",
quic::ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
quic_connection_ = nullptr;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase {
void noDelay(bool /*enable*/) override {
// No-op. TCP_NODELAY doesn't apply to UDP.
}
void setDelayedCloseTimeout(std::chrono::milliseconds timeout) override;
void readDisable(bool /*disable*/) override { NOT_REACHED_GCOVR_EXCL_LINE; }
void detectEarlyCloseWhenReadDisabled(bool /*value*/) override { NOT_REACHED_GCOVR_EXCL_LINE; }
bool readEnabled() const override { return true; }
Expand Down Expand Up @@ -105,6 +104,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase {

void closeConnectionImmediately() override;

virtual bool hasDataToWrite() PURE;

EnvoyQuicConnection* quic_connection_;

private:
Expand Down
Loading