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.
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:
Remove __thread from bwlimit variables (bug #1 — bwlimit broken with -m)
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)
All three issues from the previous review are fixed:
__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.
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.
[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`.
[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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
New --bwlimit flag that limits transfer rate using a token bucket algorithm.
(64 KB max chunk size, nanosleep-based deficit compensation)
All 7 unit tests and 31 integration tests pass with zero warnings.
Code Review: Bandwidth throttling
Token bucket in
send_n_datais a clean approach. Below are the issues I found.CRITICAL
1.
__threadbreaks bwlimit for multithreaded transfersprotocol.c:10-12io_bwlimit,bw_tokens, andbw_last_refillare all declared__thread(thread-local storage). The main thread callsio_set_bwlimit()during CLI parsing, setting its own thread-local copy. But in the multithreaded client path (-m),send_chunks_multithreadedruns in a separate thread created viathrd_create. That thread has its own independentio_bwlimit = 0from the static initializer — it never sees the value set by the main thread.Result:
--bwlimitis silently ignored whenever-mis used.Fix: change the three bw variables from
__threadto plainstatic. This works for both models:fork()copies the entire address space, so each child gets its own copy of static variables — no sharing issue.send_n_data, so there's no contention onbw_tokens/bw_last_refill— no mutex needed.BUGS
2.
--bwlimit 0is accepted but does nothingclient_cli.c:132strtoullparses0fine,io_set_bwlimit(0)setsio_bwlimit = 0, andbw_throttleimmediately returns onio_bwlimit == 0. The user gets a log message "Set bandwidth limit to 0 KB/s" but no throttling happens. Should validatekbps > 0and error on0.3.
nanosleepEINTR not handledprotocol.c:49If a signal (e.g., SIGCHLD in the fork model) interrupts
nanosleep, it returns early with the remaining time inrmtp(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-NULLrmtpand retrying would be more correct.DESIGN
4. Token bucket cap is
io_bwlimit(1 second) — prevents burst accumulationprotocol.c:39if (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_throttleis called only insend_n_data.receive_n_datais 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.pyThe test runs a transfer with
--bwlimit 10240and 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 addsSSL_writeas an alternative towrite(). Bwlimit adds chunk capping before thewrite()call. When both are merged, the chunk cap needs to apply toSSL_writetoo. Not a bug in either PR alone, but worth coordinating.Summary
Must-fix before merging:
__threadfrom bwlimit variables (bug #1 — bwlimit broken with-m)--bwlimitvalue > 0 (bug #2)Nice-to-have:
3. Handle
nanosleepEINTR (issue #3)4. Strengthen test to verify actual throttling (issue #6)
Re-review of
3dcaf0bAll three issues from the previous review are fixed:
__threadremoved from bwlimit variables —io_bwlimit,bw_tokens,bw_last_refillare now plainstatic. Works correctly in both fork model (server) and thread model (client) since only one thread at a time callssend_n_data.--bwlimit 0validation — clean error message, exits early.nanosleepEINTR handling — proper retry loop withremainingtimespec.No new issues introduced. LGTM.
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 modeio_bwlimit,bw_tokens, andbw_last_refillare all__thread(thread-local). Whenmain()callsio_set_bwlimit(), it sets only the main thread's copy. Whensend_files_multithreaded()spawns worker threads, each new thread gets its own copy initialized to0. Thebw_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:
bwlimitfield toConfig, and callio_set_bwlimit(config->bwlimit)at the top ofsend_chunks_multithreaded()Warnings
[2]
src/shared/protocol.c:20— bw_tokens initialized to 0 (no burst tolerance)bw_tokensstarts 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 * 1024strtoullwithNULLendptr 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 validateendand range.[4]
src/shared/protocol.c:43— nanosleep return value not checkedIf
nanosleepis 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 overflowCasting
io_bwlimit(unsigned long long) tolong longis UB if it exceedsLLONG_MAX. Consider usingunsigned long longforbw_tokens.Style
[6]
src/shared/protocol.c:39,42,46— Repeated cast ofio_bwlimittolong long. Usingunsigned long longforbw_tokenswith a separate signed variable for deficit computation would be cleaner.The critical issue is the most important: the
__threadstorage silently disables bandwidth limiting for all multithreaded transfers. This is a functional bug that users will hit immediately when combining-mwith--bwlimit.Re-review after commit
3dcaf0bVerdict: PASS — Critical issue fixed
Critical — resolved
io_bwlimit,bw_tokens,bw_last_refillare no longer__thread✅Throttling now works correctly in multithreaded mode.
Additional fixes
nanosleepnow handlesEINTRwith retry loop ✅--bwlimitvalidates input > 0 ✅Remaining warnings (non-blocking)
bw_tokensstill initialized to 0 — first write always sleeps. Consider initializing toio_bwlimitfor burst tolerance.strtoullwithNULLendptr — non-numeric input silently returns 0 (now caught by >0 check), but overflow from huge values still possible. Consider using&endand validating.(long long)io_bwlimitcast could overflow if rate > LLONG_MAX. Consider usingunsigned long longforbw_tokens.These are minor and non-blocking. Safe to merge.