Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: advanced split options [BREAKING] #311
feat: advanced split options [BREAKING] #311
Changes from 7 commits
b6e887a
230c2fe
81f3f2a
e283a16
d2a4cf9
5adbe71
4801a03
cb7d702
637628f
3354513
404aa65
b3bdc9e
faffebb
cae6dd4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
I'll try to run this on my machine (I don't remember the details about the test we did IRL)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For TCP sockets, Go will try to send packets ASAP without delays: https://pkg.go.dev/net#TCPConn.SetNoDelay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added 100us sleep. I wonder if that's enough or if you need a full millisecond.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea, likely some kind of congestion control kicking it.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linux
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?)
sure, will do that today in the evening