Opened 6 years ago

Last modified 4 years ago

#1040 open defect

Streaming half-close unused and broken

Reported by: zzz Owned by:
Priority: minor Milestone: eventually
Component: streaming Version: 0.9.7.1
Keywords: docs reliability Cc: Zlatin Balevsky
Parent Tickets: Sensitive: no

Description

Docs claim you can send a SYN and FIN in one packet (SYN and CLOSE in streaming terminology).

But we don't now. And it wouldn't work well if we did. If you sent a CLOSE it would start disconnect timers, e.g. Connection.setCloseSentOn(). Also, streams closed in one direction may cause closes in the other, e.g. in i2ptunnel stream forwarders.

So sending a CLOSE one direction but having the conn stay open for hours in the other direction does not work now.

Fixing this could eliminate the transmission of one packet (the CLOSE)

Subtickets

Change History (10)

comment:1 Changed 6 years ago by Zlatin Balevsky

Cc: Zlatin Balevsky added

comment:2 Changed 6 years ago by zzz

Maybe it isn't unused but it sure looks broken. Following code from 2005 in ConnectionPacketHandler?:

        if ( (con.getCloseSentOn() > 0) && (con.getUnackedPacketsSent() <= 0) && 
             (packet.getSequenceNum() > 0) && (packet.getPayloadSize() > 0)) {
            if (_log.shouldLog(Log.WARN))
                _log.warn("Received new data when we've sent them data and all of our data is acked: " 
                          + packet + " on " + con + "");
            con.sendReset();
            con.disconnect(false);
            packet.releasePayload();
            return;
        }

That looks horribly wrong.

Following shows a close sent (initiated by browser close) followed by us not acking retransmissions.

http://pastethis.i2p/show/AsaD9tGIr0VOhiCXgmsp/

Following shows us not getting our close acked:

http://pastethis.i2p/show/uR27CEYy18Mh3l5NjGKo/

Note that both pastes are from modified code, but I think it could happen in trunk. For further research.

comment:3 Changed 6 years ago by zzz

Close never acked, never retransmitted?

http://pastethis.i2p/show/wn0L7CWHngO0a7K68TD7/

comment:4 Changed 6 years ago by Zlatin Balevsky

What happens when both sides send the first close at almost the same time? For example, Alice sends her close while Bob's close is in flight.

comment:5 Changed 6 years ago by zzz

You mean how does it work now? Or with the #1042 fixes? Or after complete half-close implementation?

Each side keeps a local state. On each side, the important thing is whether you received a close before you sent a close, or not. Strictly speaking, it is whether the close you sent contained an ack of their close. Which it either did or it didn't.

In the #1042 fixes I'm not tracking that, just looking at before or after, so it could be better. But by being conservative about when to mark our close as sent, you can be pretty safe. You can always go down the left side of the 793 pic (to TIME-WAIT) if you aren't sure. The right side is just an optimization.

comment:6 Changed 6 years ago by zzz

#1042 fixes checked in to i2p.i2p.zzz.test2 33d2af1d4e98089e8f260804254f5b5bca1dd0d5 fix many half-close bugs on the receive side. Probably more to do.

To actually set CLOSE in the SYN packet (or any last packet with data) requires changes in how MessageOutputStream?.close() propagates to ConnectionDataReceiver?.buildPacket(). Right now, MessageOutputStream?.getClosed() doesn't return true until MessageOutputStream?.destroy() is called. That's what ConnectionDataReceiver?.buildPacket() calls. It should just propagate through like an EOF.

Possibly related: #629

comment:11 Changed 6 years ago by zzz

Major changes propped as 0.9.8.1-2. More to do.

comment:12 Changed 6 years ago by zzz

Keywords: docs added

Docs should be fixed at http://www.i2p2.i2p/streaming to say the originator should NOT put the CLOSE in the SYN packet. Should be OK for the receiver. Right now we say in several places that you can combine SYN and CLOSE without a lot of detail.

As there's at least two people implementing streaming in C now we should get on this.

comment:13 Changed 4 years ago by str4d

Keywords: reliability added
Milestone: 0.9.12eventually

comment:14 Changed 4 years ago by str4d

Status: newopen
Note: See TracTickets for help on using tickets.