Bug 5056: assertion failed: Read.cc:61: Comm::IsConnOpen(conn)

[eduard-bagdasaryan]

FATAL: assertion failed: Read.cc:61: "Comm::IsConnOpen(conn)"

comm_read() was called on a closed connection because it was initiated
by tunnelDelayedServerRead() event triggered after the connection
closure.

[eduard-bagdasaryan]

I checked that this assertion is valid: copyRead() is called via writeServerDone() -> copyClientBytes(), and client.dataSent() in writeServerDone() resets this field. Also for the initial case (when copyRead() is not called via writeServerDone()), copyClientBytes() takes care about the 'pre read data' in a different execution path, and from.len is not checked in this case. So I think the code below can simply close the 'other' connection, we don't need finishWritingAndDelete() with its "waiting to finish writing" logic there.

[rousskov]

I checked that this assertion is valid: copyRead() is called via ...

AFAICT, you have checked that this assertion is not going to fail in current code. I see several red flags in the description of those checks, but let's assume that it does not fail indeed. That is great, but it does not help me understand how new code (placed below this assertion) relies on the asserted condition...

... [complex reasoning about from.len value and possibly other conditions] ... So I think the code below can simply close the 'other' connection, we don't need finishWritingAndDelete() with its "waiting to finish writing" logic there.

I am having trouble connecting the dots here. Would the following statement be an accurate representation of proposed/new code logic?

• Zero from.len implies that we are not currently writing to toConn. Thus, finishWritingAndDelete(toConn) is unnecessary and toConn->close() is sufficient to correctly end transaction and destroy TunnelStateData.

N.B. Official comm_read() calling code below the assertion relies on the asserted condition because it uses a pointer to the beginning of from.buf buffer. The number of currently buffered bytes must be zero for comm_read() to safely fill that area of the buffer. From that code point of view, the assertion is correct (i.e. it checks the correct invariant that the code below the assertion relies on) even if it fails (due to Squid bugs elsewhere)!

[eduard-bagdasaryan]

Zero from.len implies that we are not currently writing to toConn.

We may imply that: from.len becomes non-zero when we have read some new data and are going to write it (Connection::bytesIn() in TunnelStateData::readClient()). It becomes zero again when we have written all available data (Connection::dataSent() asserts on that) and are going to read again (our TunnelStateData::copyRead()). So, len==0 here means that we have written all available data (if any) and do not have new data to write (yet). So when we observe that client has gone (no new data) at this point, we simply close the opposite connection.

[rousskov]

I checked that this assertion is valid: copyRead() is called via ...

AFAICT, you have checked that this assertion is not going to fail in current code. I see several red flags in the description of those checks, but let's assume that it does not fail indeed. That is great, but it does not help me understand how new code (placed below this assertion) relies on the asserted condition...

... [complex reasoning about from.len value and possibly other conditions] ... So I think the code below can simply close the 'other' connection, we don't need finishWritingAndDelete() with its "waiting to finish writing" logic there.

I am having trouble connecting the dots here. Would the following statement be an accurate representation of proposed/new code logic?

• Zero from.len implies that we are not currently writing to toConn. Thus, finishWritingAndDelete(toConn) is unnecessary and toConn->close() is sufficient to correctly end transaction and destroy TunnelStateData.

N.B. Official comm_read() calling code below the assertion relies on the asserted condition because it uses a pointer to the beginning of from.buf buffer. The number of currently buffered bytes must be zero for comm_read() to safely fill that area of the buffer. From that code point of view, the assertion is correct (i.e. it checks the correct invariant that the code below the assertion relies on) even if it fails (due to Squid bugs elsewhere)!

[rousskov]

If from.conn is not open anymore, then why was not our connection closure callback called? And if it was called, why do we need to do some extra steps here/now to end transaction and destroy TunnelStateData?

[eduard-bagdasaryan]

Perhaps we can rely on our connection closure callback and simply return here. However, it looks like that current tunnel code is inconsistent in this respect. For example, TunnelStateData::writeServerDone() does not rely on the callback (under the "If the other end has closed, so should we" comment) but TunnelStateData::readClient() does (under the "close handlers will tidy up for us" comment). I could not find a place whether/where we remove close callbacks in tunnel.cc. So it looks like that "relying on close handlers" is the correct approach.