Skip to content
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

Add an optional delay between split parts #277

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Trigger239
Copy link

@Trigger239 Trigger239 commented Mar 17, 2025

I discovered that the behavior of ByeDPi depends on the presence of the option --debug 1 in the command line arguments. This happens on my two Windows 7 machines. This is my command line:

ciadpi.exe  --debug 1 -U -T2 -npakistan.gov.pk -Hrussia-youtube.txt -s1 -q1 -An -Hrussia-blacklist.txt -s3 -o5+s -An -H:discord.com -s1 -o1 -At -f-1 -r3+s -As

If I remove --debug 1 from it, videos on YouTube will no longer play. The YouTube website itself and many other blocked websites, however, are still loaded fine. This is the exact same behavior which is observed when the arguments -Hrussia-youtube.txt -s1 -q1 -An are removed. So the combination -s1 -q1 works only when there is also --debug 1.

I'm pretty confident that --debug 1 helps because it introduces an additional delay between the parts of a split packed. This could explain why the program doesn't work when started as a Windows service (see #271): there is no stdout in that case, debug logging takes almost no time, and this is why --debug 1 makes no difference.

Probably when there are no delays caused by logging, the next call to send() happens before all the bytes from the previous call have been sent out through the socket. There is a function sock_has_notsent() implemented for Linux, but there is no such function for Windows.

In this PR I've implemented a workaround for this issue. An optional parameter [,delay_ms] in added to pos_t in command line arguments. When this parameter is specified and is greater then zero, the next part will be delayed by delay_ms milliseconds. The delay is not applied after the last part (if there is no more data to send).

When -s1 -q1 is replaced with -s1,5 -q1 (which means "add 5 ms delay after sending the first byte"), ByeDPI allows to access YouTube and play videos there. It works as a service too.

I'm not sure if this feature can be helpful for somebody else, or if it solves the problem only in my specific use case (the combination of the OS, hardware, network topology and ISP). I would appreciate it if someone else could test it and say that it helped them.

@hufrea
Copy link
Owner

hufrea commented Mar 17, 2025

Adding a new parameter for a position seems redundant. I think we can restore old behavior and make the delay global (649ec06).

@Trigger239
Copy link
Author

Trigger239 commented Mar 17, 2025

Didn't know there was a global delay at some point. Actually, the first thing I tried to do was to add Sleep(1), and that helped with YouTube. :)

For me, the global delay doesn't seem right in the event loop-based code. But feel free to reject this PR if you think a new parameter is not needed.

@hufrea
Copy link
Owner

hufrea commented Mar 17, 2025

global delay doesn't seem right in the event loop-based code

I meant the global delay parameter. The delay can be implemented using set_timer, as it is done now.

@Trigger239
Copy link
Author

Trigger239 commented Mar 17, 2025

Oh, OK. Then I should think about how to fix the infinite loop issue.

Btw, I've also tried to implement sock_has_notsent() for Windows using SetPerTcpConnectionEStats() and GetPerTcpConnectionEStats() (docs) to get the TCP_ESTATS_SEND_BUFF_ROD_v0 structure (docs). But for some reason it doesn't work at all. SetPerTcpConnectionEStats() succeeds, but the collection of TCP statistics remains disabled.

Add optional parameter [,delay_ms] to pos_t in command line arguments. When
this parameter is specified and is greater then zero, the next part is delayed
by delay_ms milliseconds.

Workaround for hufrea#271
@Trigger239
Copy link
Author

I think, the infinite loop issue is solved now. This is a completely different approach. The main idea is to keep track of the last part (including repetitions) that was sent.

Have't replace the per-part delay parameter with a global one yet, but that's not a big deal. I'll do it if everything else is fine.

@hufrea
Copy link
Owner

hufrea commented Mar 24, 2025

I added a count of all sent chunks, this fixes ignoring segments when specifying range (--oob 1:3:0). Also made the delay a global option. It is not documented yet, maybe later there will be a way to do without it.
https://github.com/hufrea/byedpi/tree/split-delay

This reverts commit 7561bb6.
This reverts commit dd4ee51.
This reverts commit 02daba5.
@Trigger239
Copy link
Author

Your version works fine, and the code is much cleaner! But there is still one more edge case:

> ciadpi --debug 1 --wait-send -H:www.http2demo.io --proto http --oob 0:3:0+ne

new conn: fd=276, pair=256, addr=195.181.172.3:80
desync TCP: group=0, round=1, fd=276
host: www.http2demo.io (22)
split: pos=0-513 (513), m: DESYNC_OOB
split: pos=513-513 (0), m: DESYNC_OOB
split: pos=513-513 (0), m: DESYNC_OOB
close: fd=276 (pair=256), recv: 808, rounds: 1

There is no delay between the chunks here. This is because the chunks have a length of 0, and they are located at the end of the packet. The result of the comparison lp < n is false, thus no delay is added.

@Trigger239
Copy link
Author

Trigger239 commented Mar 24, 2025

Should the behavior of things like --oob 0:3:0+ne be fixed? I think I have a fix for that, but I'm not sure if it is worth applying. Probably nobody will ever use such parameters.

@hufrea
Copy link
Owner

hufrea commented Mar 24, 2025

We can keep it as is. Since the returned size == n, desync will not be called again even after the timer, since all data has been sent. This can be bypassed, but it will be dirty.

@Trigger239
Copy link
Author

Well, it is not that dirty... Please have a look: Trigger239@a4358ce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants