TapTap/FastSync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bandwidth throttling: --bwlimit <KB/s> with token bucket #15

Merged
TapTap merged 3 commits from bwlimit into main 2026-07-17 10:22:46 +02:00
Conversation 4 Commits 3 Files Changed 4
+76 -1
TapTap commented 2026-07-16 16:29:05 +02:00
Owner
Copy Link
Copy Source

New --bwlimit flag that limits transfer rate using a token bucket algorithm.

  • protocol.h/c: io_set_bwlimit() function + token bucket in send_n_data
    (64 KB max chunk size, nanosleep-based deficit compensation)
  • client_cli.c: --bwlimit <KB/s> flag (value converted to bytes/s internally)
  • test.py: bandwidth limit test case (31 tests total)

All 7 unit tests and 31 integration tests pass with zero warnings.

New --bwlimit flag that limits transfer rate using a token bucket algorithm. - protocol.h/c: io_set_bwlimit() function + token bucket in send_n_data (64 KB max chunk size, nanosleep-based deficit compensation) - client_cli.c: --bwlimit <KB/s> flag (value converted to bytes/s internally) - test.py: bandwidth limit test case (31 tests total) All 7 unit tests and 31 integration tests pass with zero warnings.
TapTap added 1 commit 2026-07-16 16:29:05 +02:00
Bandwidth throttling: --bwlimit <KB/s> with token bucket 09a76e2a9a 
- protocol.h/c: io_set_bwlimit() + token bucket in send_n_data
  (64KB chunks, nanosleep-based deficit compensation)
- client_cli.c: --bwlimit <KB/s> flag
- test.py: bandwidth limit test case
TapTap commented 2026-07-16 16:55:39 +02:00
Author
Owner
Copy Link
Copy Source

Code Review: Bandwidth throttling

Token bucket in send_n_data is a clean approach. Below are the issues I found.

CRITICAL

1. __thread breaks bwlimit for multithreaded transfers
protocol.c:10-12

io_bwlimit, bw_tokens, and bw_last_refill are all declared __thread (thread-local storage). The main thread calls io_set_bwlimit() during CLI parsing, setting its own thread-local copy. But in the multithreaded client path (-m), send_chunks_multithreaded runs in a separate thread created via thrd_create. That thread has its own independent io_bwlimit = 0 from the static initializer — it never sees the value set by the main thread.

Result: --bwlimit is silently ignored whenever -m is used.

Fix: change the three bw variables from __thread to plain static. This works for both models:

  • Fork model (server): fork() copies the entire address space, so each child gets its own copy of static variables — no sharing issue.
  • Thread model (client): only the sender thread calls send_n_data, so there's no contention on bw_tokens/bw_last_refill — no mutex needed.

BUGS

2. --bwlimit 0 is accepted but does nothing
client_cli.c:132

strtoull parses 0 fine, io_set_bwlimit(0) sets io_bwlimit = 0, and bw_throttle immediately returns on io_bwlimit == 0. The user gets a log message "Set bandwidth limit to 0 KB/s" but no throttling happens. Should validate kbps > 0 and error on 0.

3. nanosleep EINTR not handled
protocol.c:49

If a signal (e.g., SIGCHLD in the fork model) interrupts nanosleep, it returns early with the remaining time in rmtp (which is NULL here). The throttle under-sleeps, resulting in slightly higher bandwidth usage. For a file transfer tool this is minor, but passing a non-NULL rmtp and retrying would be more correct.

DESIGN

4. Token bucket cap is io_bwlimit (1 second) — prevents burst accumulation
protocol.c:39

if (bw_tokens > (long long)io_bwlimit) bw_tokens = (long long)io_bwlimit; caps tokens at 1 second worth. This means after a long idle period (e.g., while the server processes metadata), the client can only burst 1 second of data. This is standard token bucket behavior and is fine, but worth documenting — some users expect idle time to "bank" bandwidth for large transfers.

5. Bwlimit only applies to sends, not receives

bw_throttle is called only in send_n_data. receive_n_data is not throttled. This is correct (the client is the sender), but should be documented. If the server ever needs to send data back (e.g., for a pull mode), it won't be throttled.

MINOR

6. Test doesn't verify bandwidth limiting
test.py

The test runs a transfer with --bwlimit 10240 and checks the exit code. It doesn't verify the transfer was actually slower. A more robust test would compare transfer time against a no-bwlimit baseline and assert a minimum duration.

7. Merge conflict with TLS PR

Both this PR and the TLS PR modify send_n_data. TLS adds SSL_write as an alternative to write(). Bwlimit adds chunk capping before the write() call. When both are merged, the chunk cap needs to apply to SSL_write too. Not a bug in either PR alone, but worth coordinating.

Summary

Must-fix before merging:

  1. Remove __thread from bwlimit variables (bug #1 — bwlimit broken with -m)
  2. Validate --bwlimit value > 0 (bug #2)

Nice-to-have:
3. Handle nanosleep EINTR (issue #3)
4. Strengthen test to verify actual throttling (issue #6)

## Code Review: Bandwidth throttling Token bucket in `send_n_data` is a clean approach. Below are the issues I found. --- ### CRITICAL **1. `__thread` breaks bwlimit for multithreaded transfers** `protocol.c:10-12` `io_bwlimit`, `bw_tokens`, and `bw_last_refill` are all declared `__thread` (thread-local storage). The main thread calls `io_set_bwlimit()` during CLI parsing, setting its own thread-local copy. But in the multithreaded client path (`-m`), `send_chunks_multithreaded` runs in a separate thread created via `thrd_create`. That thread has its own independent `io_bwlimit = 0` from the static initializer — it never sees the value set by the main thread. Result: `--bwlimit` is silently ignored whenever `-m` is used. Fix: change the three bw variables from `__thread` to plain `static`. This works for both models: - **Fork model (server):** `fork()` copies the entire address space, so each child gets its own copy of static variables — no sharing issue. - **Thread model (client):** only the sender thread calls `send_n_data`, so there's no contention on `bw_tokens`/`bw_last_refill` — no mutex needed. --- ### BUGS **2. `--bwlimit 0` is accepted but does nothing** `client_cli.c:132` `strtoull` parses `0` fine, `io_set_bwlimit(0)` sets `io_bwlimit = 0`, and `bw_throttle` immediately returns on `io_bwlimit == 0`. The user gets a log message "Set bandwidth limit to 0 KB/s" but no throttling happens. Should validate `kbps > 0` and error on `0`. **3. `nanosleep` EINTR not handled** `protocol.c:49` If a signal (e.g., SIGCHLD in the fork model) interrupts `nanosleep`, it returns early with the remaining time in `rmtp` (which is NULL here). The throttle under-sleeps, resulting in slightly higher bandwidth usage. For a file transfer tool this is minor, but passing a non-NULL `rmtp` and retrying would be more correct. --- ### DESIGN **4. Token bucket cap is `io_bwlimit` (1 second) — prevents burst accumulation** `protocol.c:39` `if (bw_tokens > (long long)io_bwlimit) bw_tokens = (long long)io_bwlimit;` caps tokens at 1 second worth. This means after a long idle period (e.g., while the server processes metadata), the client can only burst 1 second of data. This is standard token bucket behavior and is fine, but worth documenting — some users expect idle time to "bank" bandwidth for large transfers. **5. Bwlimit only applies to sends, not receives** `bw_throttle` is called only in `send_n_data`. `receive_n_data` is not throttled. This is correct (the client is the sender), but should be documented. If the server ever needs to send data back (e.g., for a pull mode), it won't be throttled. --- ### MINOR **6. Test doesn't verify bandwidth limiting** `test.py` The test runs a transfer with `--bwlimit 10240` and checks the exit code. It doesn't verify the transfer was actually slower. A more robust test would compare transfer time against a no-bwlimit baseline and assert a minimum duration. **7. Merge conflict with TLS PR** Both this PR and the TLS PR modify `send_n_data`. TLS adds `SSL_write` as an alternative to `write()`. Bwlimit adds chunk capping before the `write()` call. When both are merged, the chunk cap needs to apply to `SSL_write` too. Not a bug in either PR alone, but worth coordinating. --- ### Summary Must-fix before merging: 1. Remove `__thread` from bwlimit variables (bug #1 — bwlimit broken with `-m`) 2. Validate `--bwlimit` value > 0 (bug #2) Nice-to-have: 3. Handle `nanosleep` EINTR (issue #3) 4. Strengthen test to verify actual throttling (issue #6)
TapTap added 1 commit 2026-07-16 17:00:17 +02:00
Fix bwlimit review: remove __thread (breaks -m), validate >0, handle nanosleep EINTR 3dcaf0b79d
TapTap commented 2026-07-16 17:10:02 +02:00
Author
Owner
Copy Link
Copy Source

Re-review of 3dcaf0b

All three issues from the previous review are fixed:

  1. __thread removed from bwlimit variables — io_bwlimit, bw_tokens, bw_last_refill are now plain static. Works correctly in both fork model (server) and thread model (client) since only one thread at a time calls send_n_data.
  2. --bwlimit 0 validation — clean error message, exits early.
  3. nanosleep EINTR handling — proper retry loop with remaining timespec.

No new issues introduced. LGTM.

## Re-review of 3dcaf0b All three issues from the previous review are fixed: 1. **`__thread` removed from bwlimit variables** — `io_bwlimit`, `bw_tokens`, `bw_last_refill` are now plain `static`. Works correctly in both fork model (server) and thread model (client) since only one thread at a time calls `send_n_data`. 2. **`--bwlimit 0` validation** — clean error message, exits early. 3. **`nanosleep` EINTR handling** — proper retry loop with `remaining` timespec. No new issues introduced. LGTM.
TapTap commented 2026-07-16 17:21:05 +02:00
Author
Owner
Copy Link
Copy Source

PR Review Summary

Verdict: FAIL — 1 critical issue must be fixed before merge

Files reviewed: 3
Issues found: 6 (1 critical, 4 warning, 1 style)

CRITICAL Issue

[1] src/shared/protocol.c:14-16 / src/client/client_cli.c:132 — throttling ignored in multithreaded mode

io_bwlimit, bw_tokens, and bw_last_refill are all __thread (thread-local). When main() calls io_set_bwlimit(), it sets only the main thread's copy. When send_files_multithreaded() spawns worker threads, each new thread gets its own copy initialized to 0. The bw_throttle() check (if (io_bwlimit == 0) return;) means throttling is completely disabled in the sender worker thread.

A user running fastsync -m --bwlimit 10240 ... gets zero throttling.

Fix options:

  • (a) Add a bwlimit field to Config, and call io_set_bwlimit(config->bwlimit) at the top of send_chunks_multithreaded()
  • (b) Pass the rate through the pipeline context
  • (c) Make the throttle state non-thread-local (not recommended for hot path)

Warnings

[2] src/shared/protocol.c:20 — bw_tokens initialized to 0 (no burst tolerance)
bw_tokens starts at 0, so the first write always sleeps. Standard token bucket implementations initialize tokens to the max rate (full bucket) to allow an initial burst.
Fix: Change to bw_tokens = (long long)io_bwlimit; so the bucket starts full.

[3] src/client/client_cli.c:133 — no overflow check on kbps * 1024
strtoull with NULL endptr silently returns 0 for non-numeric input. If the user provides a value > ULLONG_MAX / 1024, the multiplication wraps.
Fix: Use strtoull(argv[++i], &end, 10) and validate end and range.

[4] src/shared/protocol.c:43 — nanosleep return value not checked
If nanosleep is interrupted by a signal, the actual sleep is shorter, allowing more data through than the rate limit intends. A retry loop would be more correct.

[5] src/shared/protocol.c:39 — potential signed overflow
Casting io_bwlimit (unsigned long long) to long long is UB if it exceeds LLONG_MAX. Consider using unsigned long long for bw_tokens.

Style

[6] src/shared/protocol.c:39,42,46 — Repeated cast of io_bwlimit to long long. Using unsigned long long for bw_tokens with a separate signed variable for deficit computation would be cleaner.

The critical issue is the most important: the __thread storage silently disables bandwidth limiting for all multithreaded transfers. This is a functional bug that users will hit immediately when combining -m with --bwlimit.

## PR Review Summary **Verdict: FAIL** — 1 critical issue must be fixed before merge **Files reviewed:** 3 **Issues found:** 6 (1 critical, 4 warning, 1 style) --- ### CRITICAL Issue **[1] `src/shared/protocol.c:14-16` / `src/client/client_cli.c:132` — throttling ignored in multithreaded mode** `io_bwlimit`, `bw_tokens`, and `bw_last_refill` are all `__thread` (thread-local). When `main()` calls `io_set_bwlimit()`, it sets only the main thread's copy. When `send_files_multithreaded()` spawns worker threads, each new thread gets its own copy initialized to `0`. The `bw_throttle()` check (`if (io_bwlimit == 0) return;`) means **throttling is completely disabled** in the sender worker thread. A user running `fastsync -m --bwlimit 10240 ...` gets **zero throttling**. **Fix options:** - (a) Add a `bwlimit` field to `Config`, and call `io_set_bwlimit(config->bwlimit)` at the top of `send_chunks_multithreaded()` - (b) Pass the rate through the pipeline context - (c) Make the throttle state non-thread-local (not recommended for hot path) --- ### Warnings **[2] `src/shared/protocol.c:20` — bw_tokens initialized to 0 (no burst tolerance)** `bw_tokens` starts at 0, so the first write always sleeps. Standard token bucket implementations initialize tokens to the max rate (full bucket) to allow an initial burst. **Fix:** Change to `bw_tokens = (long long)io_bwlimit;` so the bucket starts full. **[3] `src/client/client_cli.c:133` — no overflow check on kbps * 1024** `strtoull` with `NULL` endptr silently returns 0 for non-numeric input. If the user provides a value > `ULLONG_MAX / 1024`, the multiplication wraps. **Fix:** Use `strtoull(argv[++i], &end, 10)` and validate `end` and range. **[4] `src/shared/protocol.c:43` — nanosleep return value not checked** If `nanosleep` is interrupted by a signal, the actual sleep is shorter, allowing more data through than the rate limit intends. A retry loop would be more correct. **[5] `src/shared/protocol.c:39` — potential signed overflow** Casting `io_bwlimit` (unsigned long long) to `long long` is UB if it exceeds `LLONG_MAX`. Consider using `unsigned long long` for `bw_tokens`. ### Style **[6] `src/shared/protocol.c:39,42,46`** — Repeated cast of `io_bwlimit` to `long long`. Using `unsigned long long` for `bw_tokens` with a separate signed variable for deficit computation would be cleaner. --- The critical issue is the most important: the `__thread` storage silently disables bandwidth limiting for all multithreaded transfers. This is a functional bug that users will hit immediately when combining `-m` with `--bwlimit`.
TapTap commented 2026-07-17 09:57:50 +02:00
Author
Owner
Copy Link
Copy Source

Re-review after commit 3dcaf0b

Verdict: PASS — Critical issue fixed

Critical — resolved

  • [1] io_bwlimit, bw_tokens, bw_last_refill are no longer __thread ✅
    Throttling now works correctly in multithreaded mode.

Additional fixes

  • nanosleep now handles EINTR with retry loop ✅
  • --bwlimit validates input > 0 ✅

Remaining warnings (non-blocking)

  • [2] bw_tokens still initialized to 0 — first write always sleeps. Consider initializing to io_bwlimit for burst tolerance.
  • [3] strtoull with NULL endptr — non-numeric input silently returns 0 (now caught by >0 check), but overflow from huge values still possible. Consider using &end and validating.
  • [5] (long long)io_bwlimit cast could overflow if rate > LLONG_MAX. Consider using unsigned long long for bw_tokens.

These are minor and non-blocking. Safe to merge.

## Re-review after commit `3dcaf0b` **Verdict: PASS** — Critical issue fixed ### Critical — resolved - [1] `io_bwlimit`, `bw_tokens`, `bw_last_refill` are no longer `__thread` ✅ Throttling now works correctly in multithreaded mode. ### Additional fixes - `nanosleep` now handles `EINTR` with retry loop ✅ - `--bwlimit` validates input > 0 ✅ ### Remaining warnings (non-blocking) - [2] `bw_tokens` still initialized to 0 — first write always sleeps. Consider initializing to `io_bwlimit` for burst tolerance. - [3] `strtoull` with `NULL` endptr — non-numeric input silently returns 0 (now caught by >0 check), but overflow from huge values still possible. Consider using `&end` and validating. - [5] `(long long)io_bwlimit` cast could overflow if rate > LLONG_MAX. Consider using `unsigned long long` for `bw_tokens`. These are minor and non-blocking. Safe to merge.
TapTap added 1 commit 2026-07-17 10:07:23 +02:00
fix: bw_tokens initialized to io_bwlimit, overflow check for --bwlimit 4f8ae5bb70
TapTap merged commit 45dd62f568 into main 2026-07-17 10:22:46 +02:00
TapTap deleted branch bwlimit 2026-07-17 10:22:54 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Defect or bug
 enhancement
Feature or improvement
 memory
Memory safety issue
 needs-triage
Needs human review
 performance
Performance issue
 quality
Code quality improvement
 security
Security vulnerability
 threading
Threading or concurrency
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
TapTap
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: TapTap/FastSync#15
Delete Branch "bwlimit"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?