Refactor enqueueFileTransfer#420
Refactor enqueueFileTransfer#420proski wants to merge 1 commit intoDeterminateSystems:filetransfer-enqueue-exceptionsfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactored S3 request handling in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
proski
force-pushed
the
refactor-enqueueFileTransfer
branch
from
85e1dd9 to
9dd7d67
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libstore/filetransfer.cc`:
- Around line 975-991: Remove the noexcept from makeTransferItem so exceptions
from setupForS3() and make_ref<TransferItem>() can propagate into the existing
error-handling in enqueueFileTransfer; update the declaration of
makeTransferItem (the function that calls setupForS3() and returns
make_ref<TransferItem>) to be non-noexcept so thrown BadURL or parsing
exceptions are caught by the try/catch around the call in enqueueFileTransfer
and translated via item->fail(e).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82d9f7de-67d7-4752-a652-c9a12c9f1d82
📒 Files selected for processing (1)
src/libstore/filetransfer.cc
| inline ref<TransferItem> | ||
| makeTransferItem(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept | ||
| { | ||
| /* Handle s3:// URIs by converting to HTTPS and optionally adding auth */ | ||
| if (request.uri.scheme() == "s3") { | ||
| auto modifiedRequest = request; | ||
| modifiedRequest.setupForS3(); | ||
| auto item = make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback)); | ||
| try { | ||
| return enqueueItem(item); | ||
| } catch (const nix::BaseError & e) { | ||
| // NOTE(cole-h): catches both nix::Error and nix::Interrupted -- enqueueItem calls | ||
| // writeFull which may throw nix::Interrupted, and the rest of enqueueItem may throw | ||
| // nix::Error | ||
| item->fail(e); | ||
| return ItemHandle(item.get_ptr()); | ||
| } | ||
| return make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback)); | ||
| } else { | ||
| return make_ref<TransferItem>(*this, request, std::move(callback)); | ||
| } | ||
| } | ||
|
|
||
| ItemHandle | ||
| enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override | ||
| { | ||
| const auto item = makeTransferItem(request, std::move(callback)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/libstore/filetransfer.cc | sed -n '970,1010p'Repository: DeterminateSystems/nix-src
Length of output: 1870
🏁 Script executed:
rg -A 10 "void setupForS3" src/libstore/filetransfer.ccRepository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
rg "try\s*{" src/libstore/filetransfer.cc | head -20Repository: DeterminateSystems/nix-src
Length of output: 147
🏁 Script executed:
rg "setupForS3" src/libstore/filetransfer.cc -B 2 -A 15Repository: DeterminateSystems/nix-src
Length of output: 1544
🏁 Script executed:
rg "class TransferItem" src/libstore/filetransfer.cc -A 30Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
rg "make_ref" include/ -t cpp --max-count=5Repository: DeterminateSystems/nix-src
Length of output: 118
🏁 Script executed:
rg "ParsedS3URL::parse" src/ -B 2 -A 10Repository: DeterminateSystems/nix-src
Length of output: 5572
🏁 Script executed:
rg "struct TransferItem" src/libstore/filetransfer.cc -A 25Repository: DeterminateSystems/nix-src
Length of output: 1143
🏁 Script executed:
rg "TransferItem\(" src/libstore/filetransfer.cc -B 2 -A 5 | head -40Repository: DeterminateSystems/nix-src
Length of output: 1283
🏁 Script executed:
rg "TransferItem::" src/libstore/filetransfer.cc -A 20 | grep -A 20 "TransferItem("Repository: DeterminateSystems/nix-src
Length of output: 52
🏁 Script executed:
rg "noexcept" src/libstore/filetransfer.cc | grep -i "transfer"Repository: DeterminateSystems/nix-src
Length of output: 405
Remove noexcept from makeTransferItem() to allow proper error handling.
Line 976 marks makeTransferItem() as noexcept, but it calls setupForS3() and make_ref<TransferItem>(), both of which can throw. The setupForS3() method calls ParsedS3URL::parse(), which throws BadURL and other parsing exceptions. Since makeTransferItem() is invoked on line 991 before the try block (line 993), any of these exceptions will trigger std::terminate instead of being caught and translated into a failed transfer callback via item->fail(e). Remove the noexcept qualifier and ensure item construction is covered by the existing error-handling path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libstore/filetransfer.cc` around lines 975 - 991, Remove the noexcept
from makeTransferItem so exceptions from setupForS3() and
make_ref<TransferItem>() can propagate into the existing error-handling in
enqueueFileTransfer; update the declaration of makeTransferItem (the function
that calls setupForS3() and returns make_ref<TransferItem>) to be non-noexcept
so thrown BadURL or parsing exceptions are caught by the try/catch around the
call in enqueueFileTransfer and translated via item->fail(e).
proski
force-pushed
the
refactor-enqueueFileTransfer
branch
from
9dd7d67 to
b0abc42
Compare
|
Removed |
1 participant
Motivation
The
enqueueFileTransferfunction needs refactoring, even more so after PR #348.This PR (just one commit) should be added to PR #348.
Context
I tried to add the change as a suggestion, but the result did not look good in github GUI. So I'm making it a PR.
Summary by CodeRabbit
No user-facing changes in this release. This update includes internal refactoring of file transfer handling to improve code maintainability.