fix(http): fallback when OS rejects large HTTP socket buffers#994
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSocket buffer sizing now uses SocketDefaults._set_socket_buffer: it maps option names, attempts requested SO_RCVBUF/SO_SNDBUF, retries with halved sizes on ENOBUFS down to MIN_SOCKET_BUFFER_FALLBACK_BYTES while logging distinct (option, requested, fallback) once; apply_to_socket uses this and tests validate behavior. ChangesSocket buffer fallback mechanism and integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@653493e162ee6431aeb683fd86fba8a801123e3dRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@653493e162ee6431aeb683fd86fba8a801123e3dLast updated for commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aiperf/transports/http_defaults.py`:
- Around line 53-76: The code currently forces a 1024 buffer size after the loop
which overrides requested values under 1024; change the tail so instead of
unconditionally calling sock.setsockopt(..., 1024) you attempt to set the
original requested value (value) if the loop was skipped or candidate dropped
below 1024: call sock.setsockopt(socket.SOL_SOCKET, option_name, value) and
handle OSError the same way as inside the loop (only swallow errno.ENOBUFS and
fall back by halving candidate), and only fall back to 1024 if all halved
candidates fail; keep using cls._logged_buffer_fallbacks and _logger for
warnings, and reference option_name, value, candidate, and sock.setsockopt in
the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 861855c5-fc26-4bd0-85aa-67150072a754
📒 Files selected for processing (2)
src/aiperf/transports/http_defaults.pytests/unit/transports/test_tcp_connector.py
dynamo-ops
left a comment
There was a problem hiding this comment.
Previous review comments have been addressed. Approving.
dynamo-ops
left a comment
There was a problem hiding this comment.
Previous review comments have been addressed. Approving.
dynamo-ops
left a comment
There was a problem hiding this comment.
Previous review comments have been addressed. Approving.
25237d2 to
07a474e
Compare
07a474e to
e17db11
Compare
|
Actionable comments posted: 0 |
AIPerf configures 10 MiB send/receive buffers for HTTP streaming sockets by default. On macOS, the default `kern.ipc.maxsockbuf` can be lower than the requested buffer size, causing socket initialization to fail with `OSError(55, 'No buffer space available')`. The socket buffer size is a performance optimization, not a correctness requirement, so failing startup is unnecessarily strict. This change retries with smaller buffer sizes when the OS rejects the requested value with `ENOBUFS`, logs the fallback, and keeps raising unrelated socket errors Signed-off-by: ntny <ntny1986@gmail.com>
for more information, see https://pre-commit.ci
- fix: suppress repeated socket buffer fallback warnings and signed commits Signed-off-by: ntny <ntny1986@gmail.com>
e17db11 to
653493e
Compare
AIPerf configures 10 MiB send/receive buffers for HTTP streaming sockets during startup. On some macOS systems, the default
kern.ipc.maxsockbuflimit is lower than the requested size. In this case, socket initialization can fail with:This can be worked around by increasing the kernel limit:
or by lowering AIPerf's socket buffer settings via AIPERF_HTTP_SO_RCVBUF / AIPERF_HTTP_SO_SNDBUF.
Users can work around this by tuning the OS limit or overriding AIPerf's socket buffer settings, but AIPerf should also work with default OS settings. The configured socket buffer size is a throughput optimization, not a correctness requirement, so startup should not fail solely because the OS rejects the requested optimization.
This PR adds a fallback mechanism that retries socket initialization with progressively smaller buffer sizes when the OS rejects the requested value with ENOBUFS. A warning is logged once per fallback tuple, a final error is logged if even the minimum fallback size cannot be applied, and unrelated socket errors continue to be raised normally.
also adds tests covering the fallback and logging behavior.
Summary by CodeRabbit
Bug Fixes
Tests