feat: advanced split options [BREAKING] #311

fortuna · 2024-11-05T00:02:53Z

This is based on @PeterZhizhin's PR #309

Adds a way to specify a sequence of splits.

I found it was easier for me to make the changes I was asking than asking him to contribute them.

fortuna · 2024-11-05T00:05:41Z

~~Config changes will be on a separate PR, after this one is submitted.~~

transport/split/writer.go

derlaft · 2024-11-05T21:05:50Z

transport/split/writer.go

-		w.prefixBytes -= int64(written)
+	for 0 < w.nextSplitBytes && w.nextSplitBytes < int64(len(data)) {
+		dataToSend := data[:w.nextSplitBytes]
+		n, err := w.writer.Write(dataToSend)


What we discovered during the experiments: it's not enough to just split data into multiple writes.

OS sometimes unites the writes anyways.

smth like https://github.com/hufrea/byedpi/blob/75671fa11c57aa458e87d876efc48b40985ed78f/desync.c#L88 or just a small Sleep is required to make it reliable.

Maybe there's a better way (somehow make the OS buffer sizes smaller?)

Were the experiments using Go? I have not seen that happen. I'd appreciate a way to reproduce it before we can add and wait.

In Go each TCPConn.Write will correspond to one write system call. With the NoDelay enabled (the default), it ends up being one write consistently. Make sure you don't have TCP_CORK enabled.

Were the experiments using Go?

yes

With the NoDelay enabled (the default), it ends up being one write consistently. Make sure you don't have TCP_CORK enabled.

I'll try to run this on my machine (I don't remember the details about the test we did IRL)

For TCP sockets, Go will try to send packets ASAP without delays: https://pkg.go.dev/net#TCPConn.SetNoDelay

The thing about NoDelay: Documentation says "meaning that data is sent as soon as possible after a Write".

We don't have any guarantees that "as soon is possible" is before the next write. It might happen later.

I took 4801a03, manually configured it to spit 999 times per 1 byte and see the following picture when running fetch:

00:18:59.021904 IP nyanpc.55852 > opennet.ru.http: Flags [S], seq 500399592, win 64240, options [mss 1460,sackOK,TS val 1345057543 ecr 0,nop,wscale 7], length 0 00:18:59.069457 IP opennet.ru.http > nyanpc.55852: Flags [S.], seq 4096694502, ack 500399593, win 28960, options [mss 1452,sackOK,TS val 4132463171 ecr 1345057543,nop,wscale 7], length 0 00:18:59.069476 IP nyanpc.55852 > opennet.ru.http: Flags [.], ack 1, win 502, options [nop,nop,TS val 1345057590 ecr 4132463171], length 0 00:18:59.069708 IP nyanpc.55852 > opennet.ru.http: Flags [P.], seq 1:2, ack 1, win 502, options [nop,nop,TS val 1345057590 ecr 4132463171], length 1: HTTP 00:18:59.069712 IP nyanpc.55852 > opennet.ru.http: Flags [P.], seq 2:4, ack 1, win 502, options [nop,nop,TS val 1345057590 ecr 4132463171], length 2: HTTP 00:18:59.069720 IP nyanpc.55852 > opennet.ru.http: Flags [P.], seq 4:20, ack 1, win 502, options [nop,nop,TS val 1345057590 ecr 4132463171], length 16: HTTP 00:18:59.069770 IP nyanpc.55852 > opennet.ru.http: Flags [P.], seq 20:92, ack 1, win 502, options [nop,nop,TS val 1345057590 ecr 4132463171], length 72: HTTP 00:18:59.117256 IP opennet.ru.http > nyanpc.55852: Flags [.], ack 2, win 227, options [nop,nop,TS val 4132463219 ecr 1345057590], length 0

The first packet is len 1 (expected), but then it's 2, 16, 72.

If I add time.Sleep(time.Millisecond) after the write I see what I expect to see:

00:23:20.956161 IP nyanpc.35648 > opennet.ru.http: Flags [S], seq 3537089940, win 64240, options [mss 1460,sackOK,TS val 1345319477 ecr 0,nop,wscale 7], length 0 00:23:21.003374 IP opennet.ru.http > nyanpc.35648: Flags [S.], seq 1632357611, ack 3537089941, win 28960, options [mss 1452,sackOK,TS val 4132725105 ecr 1345319477,nop,wscale 7], length 0 00:23:21.003389 IP nyanpc.35648 > opennet.ru.http: Flags [.], ack 1, win 502, options [nop,nop,TS val 1345319524 ecr 4132725105], length 0 00:23:21.003585 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 1:2, ack 1, win 502, options [nop,nop,TS val 1345319524 ecr 4132725105], length 1: HTTP 00:23:21.004670 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 2:3, ack 1, win 502, options [nop,nop,TS val 1345319525 ecr 4132725105], length 1: HTTP 00:23:21.005743 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 3:4, ack 1, win 502, options [nop,nop,TS val 1345319526 ecr 4132725105], length 1: HTTP 00:23:21.006810 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 4:5, ack 1, win 502, options [nop,nop,TS val 1345319527 ecr 4132725105], length 1: HTTP 00:23:21.007874 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 5:6, ack 1, win 502, options [nop,nop,TS val 1345319529 ecr 4132725105], length 1: HTTP 00:23:21.008934 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 6:7, ack 1, win 502, options [nop,nop,TS val 1345319530 ecr 4132725105], length 1: HTTP 00:23:21.010007 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 7:8, ack 1, win 502, options [nop,nop,TS val 1345319531 ecr 4132725105], length 1: HTTP 00:23:21.011070 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 8:9, ack 1, win 502, options [nop,nop,TS val 1345319532 ecr 4132725105], length 1: HTTP 00:23:21.012130 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 9:10, ack 1, win 502, options [nop,nop,TS val 1345319533 ecr 4132725105], length 1: HTTP 00:23:21.013168 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 10:11, ack 1, win 502, options [nop,nop,TS val 1345319534 ecr 4132725105], length 1: HTTP 00:23:21.051078 IP opennet.ru.http > nyanpc.35648: Flags [.], ack 2, win 227, options [nop,nop,TS val 4132725152 ecr 1345319524], length 0 00:23:21.051084 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 11:46, ack 1, win 502, options [nop,nop,TS val 1345319572 ecr 4132725152], length 35: HTTP 00:23:21.051205 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 46:47, ack 1, win 502, options [nop,nop,TS val 1345319572 ecr 4132725152], length 1: HTTP 00:23:21.051745 IP opennet.ru.http > nyanpc.35648: Flags [.], ack 3, win 227, options [nop,nop,TS val 4132725153 ecr 1345319525], length 0 00:23:21.052274 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 47:48, ack 1, win 502, options [nop,nop,TS val 1345319573 ecr 4132725153], length 1: HTTP 00:23:21.052719 IP opennet.ru.http > nyanpc.35648: Flags [.], ack 4, win 227, options [nop,nop,TS val 4132725154 ecr 1345319526], length 0 00:23:21.053341 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 48:49, ack 1, win 502, options [nop,nop,TS val 1345319574 ecr 4132725154], length 1: HTTP 00:23:21.053974 IP opennet.ru.http > nyanpc.35648: Flags [.], ack 5, win 227, options [nop,nop,TS val 4132725155 ecr 1345319527], length 0 00:23:21.054405 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 49:50, ack 1, win 502, options [nop,nop,TS val 1345319575 ecr 4132725155], length 1: HTTP 00:23:21.055214 IP opennet.ru.http > nyanpc.35648: Flags [.], ack 6, win 227, options [nop,nop,TS val 4132725157 ecr 1345319529], length 0 00:23:21.055464 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 50:51, ack 1, win 502, options [nop,nop,TS val 1345319576 ecr 4132725157], length 1: HTTP 00:23:21.056222 IP opennet.ru.http > nyanpc.35648: Flags [.], ack 7, win 227, options [nop,nop,TS val 4132725158 ecr 1345319530], length 0 00:23:21.056497 IP nyanpc.35648 > opennet.ru.http: Flags [P.], seq 51:52, ack 1, win 502, options [nop,nop,TS val 1345319577 ecr 4132725158], length 1: HTTP

I added 100us sleep. I wonder if that's enough or if you need a full millisecond.

Any idea why you are getting 35 there?

No idea, likely some kind of congestion control kicking it.

I added 100us sleep. I wonder if that's enough or if you need a full millisecond.

If congestion control is causing this, relying on delays won't be good enough for mobile network with high latencies. Need some experimentation, and maybe putting the delay into config.

Porting https://github.com/hufrea/byedpi/blob/75671fa11c57aa458e87d876efc48b40985ed78f/desync.c#L88 for android should also be possible, but tricky. I wonder if this option is also supported on ios, but there's a solution for windows.

https://pkg.go.dev/net#TCPConn.SetWriteBuffer

hmm, is this what we need?

setting write buffer to a relatively low value when doing splits, then maybe restoring it

@derlaft What operating system are you using?

I get it working fine on macOS without the sleep:

FYI, I pushed the config changes. Can you try it again with the new code?

linux

I get it working fine on macOS without the sleep:

looks like mac stack is simpler in that aspect (which is good news, means we won't likely have to do anything for ios as well?)

FYI, I pushed the config changes. Can you try it again with the new code?

sure, will do that today in the evening

transport/split/writer.go

derlaft · 2024-11-05T21:24:03Z

transport/split/writer.go

+	// Bytes until the next split. This must always be > 0, unless splits are done.
+	nextSplitBytes int64


is this field needed? you could directly check remainingSplits[0] and it might save some code maintaining additional counter

It was needed because remainingSplits[0] may have count > 1, and you want to edit the bytes just for the first split, and preserve the original value for subsequent splits.

jyyi1

Thanks for the new code!

transport/split/writer.go

jyyi1 · 2024-11-05T21:59:53Z

transport/split/writer.go

-		w.prefixBytes -= int64(written)
+	for 0 < w.nextSplitBytes && w.nextSplitBytes < int64(len(data)) {
+		dataToSend := data[:w.nextSplitBytes]
+		n, err := w.writer.Write(dataToSend)


For TCP sockets, Go will try to send packets ASAP without delays: https://pkg.go.dev/net#TCPConn.SetNoDelay

transport/split/writer.go

fortuna

I revamped the code to use SplitIterator. Please take another look

transport/split/writer.go

derlaft · 2024-11-05T23:14:32Z

Does not compile for me right now:

# github.com/Jigsaw-Code/outline-sdk/x/configurl
../../configurl/split.go:37:36: cannot use int64(prefixBytes) (value of type int64) as split.SplitIterator value in argument to split.NewStreamDialer

I think your CI checks works because it uses the unmodified v0.0.17 version of the root package

fortuna · 2024-11-05T23:56:39Z

The config changes will be sent on another PR. You are probably replacing the dependency on the main module, that's why it's breaking

fortuna · 2024-11-06T00:01:14Z

Config updated. With multiple sequences!

fortuna · 2024-11-06T22:14:09Z

@jyyi1 thoughts?

jyyi1

The code looks a lot cleaner now! Thanks for the update. It'll be good to go once we fixed these small issues.

transport/split/writer.go

transport/split/stream_dialer.go

transport/split/writer.go

x/configurl/split.go

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

fortuna

Comments addressed

transport/split/writer.go

transport/split/stream_dialer.go

jyyi1

Cool! Thanks.

Advanced split

b6e887a

fortuna requested a review from jyyi1 November 5, 2024 00:02

Update comment

230c2fe

Comment

81f3f2a

derlaft reviewed Nov 5, 2024

View reviewed changes

transport/split/writer.go Outdated Show resolved Hide resolved

fortuna commented Nov 5, 2024

View reviewed changes

transport/split/writer.go Outdated Show resolved Hide resolved

Fix

e283a16

derlaft reviewed Nov 5, 2024

View reviewed changes

Support ReaderFrom

d2a4cf9

derlaft reviewed Nov 5, 2024

View reviewed changes

transport/split/writer.go Show resolved Hide resolved

fortuna requested a review from derlaft November 5, 2024 21:17

derlaft reviewed Nov 5, 2024

View reviewed changes

jyyi1 reviewed Nov 5, 2024

View reviewed changes

fortuna added 2 commits November 5, 2024 18:05

Introduce SplitIterator

5adbe71

Fix Dialer

4801a03

fortuna commented Nov 5, 2024

View reviewed changes

transport/split/writer.go Outdated Show resolved Hide resolved

transport/split/writer.go Outdated Show resolved Hide resolved

fortuna changed the title ~~feat: advanced split options~~ feat: advanced split options [BREAKING] Nov 5, 2024

Add sleep

cb7d702

Update config

637628f

fortuna added 2 commits November 5, 2024 19:33

Revert sleep

3354513

Update config

404aa65

fortuna requested review from jyyi1 and derlaft November 6, 2024 00:36

jyyi1 reviewed Nov 6, 2024

View reviewed changes

transport/split/writer.go Outdated Show resolved Hide resolved

transport/split/stream_dialer.go Show resolved Hide resolved

transport/split/writer.go Show resolved Hide resolved

x/configurl/split.go Outdated Show resolved Hide resolved

Apply suggestions from code review

b3bdc9e

Co-authored-by: J. Yi <93548144+jyyi1@users.noreply.github.com>

fortuna added 2 commits November 6, 2024 18:37

Check for nil

faffebb

Update mod

cae6dd4

fortuna commented Nov 6, 2024

View reviewed changes

transport/split/writer.go Show resolved Hide resolved

transport/split/stream_dialer.go Show resolved Hide resolved

fortuna requested a review from jyyi1 November 6, 2024 23:38

jyyi1 approved these changes Nov 7, 2024

View reviewed changes

fortuna merged commit dc0a0b0 into main Nov 7, 2024
6 checks passed

fortuna deleted the fortuna-split-1 branch November 7, 2024 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: advanced split options [BREAKING] #311

feat: advanced split options [BREAKING] #311

fortuna commented Nov 5, 2024

fortuna commented Nov 5, 2024 •

edited

Loading

derlaft Nov 5, 2024

fortuna Nov 5, 2024

derlaft Nov 5, 2024

jyyi1 Nov 5, 2024

derlaft Nov 5, 2024

fortuna Nov 5, 2024

derlaft Nov 6, 2024

derlaft Nov 6, 2024

fortuna Nov 6, 2024

derlaft Nov 6, 2024

derlaft Nov 5, 2024

fortuna Nov 5, 2024

jyyi1 left a comment

jyyi1 Nov 5, 2024

fortuna left a comment

derlaft commented Nov 5, 2024

fortuna commented Nov 5, 2024

fortuna commented Nov 6, 2024 •

edited

Loading

fortuna commented Nov 6, 2024

jyyi1 left a comment

fortuna left a comment

jyyi1 left a comment

		// Bytes until the next split. This must always be > 0, unless splits are done.
		nextSplitBytes int64

feat: advanced split options [BREAKING] #311

feat: advanced split options [BREAKING] #311

Conversation

fortuna commented Nov 5, 2024

fortuna commented Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

derlaft commented Nov 5, 2024

fortuna commented Nov 5, 2024

fortuna commented Nov 6, 2024 • edited Loading

fortuna commented Nov 6, 2024

jyyi1 left a comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

fortuna commented Nov 5, 2024 •

edited

Loading

fortuna commented Nov 6, 2024 •

edited

Loading