Skip to content

connection: always flush data on close. #13892

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

Closed
PiotrSikora wants to merge 1 commit into from
Closed

connection: always flush data on close. #13892

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: 6 additions & 6 deletions source/common/network/connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,15 @@ void ConnectionImpl::close(ConnectionCloseType type) {

uint64_t data_to_write = write_buffer_->length();
ENVOY_CONN_LOG(debug, "closing data_to_write={} type={}", *this, data_to_write, enumToInt(type));
if (data_to_write > 0) {
// Try to write as much as we can if there is pending data.
transport_socket_->doWrite(*write_buffer_, type == ConnectionCloseType::NoFlush ||
!transport_socket_->canFlushClose());
}

Copy link
Contributor

@antoniovicente antoniovicente Nov 4, 2020

Choose a reason for hiding this comment

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

Let's start with the usual feedback: please add some tests.

Going beyond that, I'm not 100% sure it makes sense to attempt to flush before close in all cases. It's possible that we don't really mean it when we currently say ConnectionCloseType::NoFlush given that the previous version of this method attempted a write when type was NoFlush. Adding a NoReallyThrowDataAwayAndClose enum type seems like overkill, but it also seems somewhat appropriate.

Copy link
Contributor Author

@PiotrSikora PiotrSikora Nov 4, 2020

Choose a reason for hiding this comment

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

Oh, I didn't indent to merge this PR as-is, I've added it here to start the discussion about possible solution for #13890. A test for it would be working FlushWrite in #13858, but that's not merged yet.

At this point I'm not really sure what the current semantics are and why NoFlush does flush data, to be honest.

const bool delayed_close_timeout_set = delayed_close_timeout_.count() > 0;
if (data_to_write == 0 || type == ConnectionCloseType::NoFlush ||
!transport_socket_->canFlushClose()) {
if (data_to_write > 0) {
// We aren't going to wait to flush, but try to write as much as we can if there is pending
// data.
transport_socket_->doWrite(*write_buffer_, true);
}

if (type == ConnectionCloseType::FlushWriteAndDelay && delayed_close_timeout_set) {
// The socket is being closed and either there is no more data to write or the data can not be
// flushed (!transport_socket_->canFlushClose()). Since a delayed close has been requested,
Expand Down