connection: always flush data on close.

[PiotrSikora]

Fixes #13890.

Signed-off-by: Piotr Sikora piotrsikora@google.com

[antoniovicente]

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.

[PiotrSikora]

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.

[ggreenway]

I agree that this should fix the behavior we were seeing, but I don't understand yet why it was failing. In the NoFlush case it was calling transport_socket_->doWrite() immediately, but in the Flush case it was queuing a writable event, and transport_socket_->doWrite() gets called there. I haven't figured out what is different in these cases. @PiotrSikora or @antoniovicente did either of you spot why this doesn't work as-is?

[PiotrSikora]

I agree that this should fix the behavior we were seeing, but I don't understand yet why it was failing. In the NoFlush case it was calling transport_socket_->doWrite() immediately, but in the Flush case it was queuing a writable event, and transport_socket_->doWrite() gets called there. I haven't figured out what is different in these cases. @PiotrSikora or @antoniovicente did either of you spot why this doesn't work as-is?

I don't see transport_socket_->doWrite() being called in the FlushWrite case.

[ggreenway]

I don't see transport_socket_->doWrite() being called in the FlushWrite case.

It's the call to enableFileEvents(Event::FileReadyType::Write), which should cause onFileEvent() to get called, which then calls onWriteReady(), which is where transport_socket_->doWrite() is called.

[PiotrSikora]

I don't see transport_socket_->doWrite() being called in the FlushWrite case.

It's the call to enableFileEvents(Event::FileReadyType::Write), which should cause onFileEvent() to get called, which then calls onWriteReady(), which is where transport_socket_->doWrite() is called.

I understand the chain of theoretical event, I'm just saying that this (transport_socket_->doWrite() being called) is not happening in practice, AFAICT.

[antoniovicente]

I don't see transport_socket_->doWrite() being called in the FlushWrite case.

It's the call to enableFileEvents(Event::FileReadyType::Write), which should cause onFileEvent() to get called, which then calls onWriteReady(), which is where transport_socket_->doWrite() is called.

I understand the chain of theoretical event, I'm just saying that this (transport_socket_->doWrite() being called) is not happening in practice, AFAICT.

I'm trying to debug what's happening, give me a few minutes.

[antoniovicente]

I don't see transport_socket_->doWrite() being called in the FlushWrite case.

It's the call to enableFileEvents(Event::FileReadyType::Write), which should cause onFileEvent() to get called, which then calls onWriteReady(), which is where transport_socket_->doWrite() is called.

I understand the chain of theoretical event, I'm just saying that this (transport_socket_->doWrite() being called) is not happening in practice, AFAICT.

I'm trying to debug what's happening, give me a few minutes.

This is caused by calling close directly from onEvent, see explanation at https://github.com/envoyproxy/envoy/pull/13858/files/ca79634eb889c3de0c51b74989d3d6704ef4265f#r517692030

[PiotrSikora]

Abandoned (see #13890).