Implement optimistic data for streams

Today's arti doesn't send optimistic data on streams. This should be an easy fix, but it'll take some hacking.

added Backlog label

added First Contribution label

removed Backlog label

unassigned @nickm

Conceivably we'll do this earlier to help with latency.

changed milestone to %Arti: Feature parity with the C implementation

changed time estimate to 20h

added Icebox label

removed First Contribution label

I'd like to have a try if no one is working on it already. I found closed proposals for optimistic data server and optimistic data client. Will read them first and ask questions here.

Sounds good!

In summary, the main idea for 'optimistic data' is that you can send data on a Tor stream immediately after you send the BEGIN cell, without having to wait for a CONNECTED cell. If an END cell arrives instead, or if the circuit was closed, then the data is dropped.

We don't want this behavior to be always-on; it should probably be configurable and network-specific.

Hi! @nickm Some questions about the implementation when you have time please:

1. Should optimistic data configurable via the ConnectPref? And this config should be passed into ClientCirc::begin_stream?
2. I saw the comment in being_data_stream. If we return a DataStream before receiving Connected, do we still need to do anything with the RawCellStream? Thanks!

@lyuyuan Hi!

1. I think that's reasonable. We'll want to refactor our APIs a bit as we move forward, but ConnectPref seems like a reasonable place to put this.

2. If a stream is optimistic, then we need to make sure that the first cell we receive on it is a CONNECTED or an END. It's a protocol error otherwise. After we get an END, we should not accept any more cells. We should not accept more than one CONNECTED on a stream. We should have some way to distinguish "END before CONNECTED received" from a regular END on a connected stream.

@nickm Hi Nick, sorry for the delay, I have been busy lately and finally got time to revisit this issue. I am planning to do the followings:

1. Implement a new method for DataReader to read a RelayMsg.
2. Move the raw cell stream here into data stream and return it to proxy, instead of waiting for response message.
3. In handle_socks_conn, based on optimistic config value, either first start copy from sock_r to tor_w right away then read RelayMsg from DataStream::r, or wait for the response first. Do you think this would work?

Another question, we currently have a hard-coded 10 seconds timeout for getting a data stream. When optimistic data is on, do we need a separate timeout for receiving response?

Thanks!

Hm. I don't want to break the abstraction on DataStream to do this: I'd rather just have an optimistic stream behave (mostly) the same as a regular DataStream.

What it should (probably) do IMO is just add an extra error type for "Used optimistic begin, but got an END instead of a CONNECTED". Then the begin_data_stream function can either wait for the CONNECTED (or not), and adjust the state of the DataStream accordingly.

That way, the only change necessary in tor-client or handle_socks_conn would be to either set optimistic data or not.

mentioned in merge request !116 (merged)

Closed with !116 (merged). Thanks again!

closed