Extract libuv ownership primitives into acyclic uv component - #8281
Open
Amaury Chamayou (achamayou) wants to merge 2 commits into
Open
Extract libuv ownership primitives into acyclic uv component#8281Amaury Chamayou (achamayou) wants to merge 2 commits into
Amaury Chamayou (achamayou) wants to merge 2 commits into
Conversation
This was referenced Sep 4, 2026
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-bookish-memory
branch
from
September 4, 2026 13:25
3da8559 to
480452a
Compare
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
September 4, 2026 13:26
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new src/uv/proxy.h is missing required standard headers for used std:: utilities, which can break builds depending on include order.
Pull request overview
This PR introduces a new internal uv source component to host the generic libuv handle-ownership primitives, removing the http -> host source dependency edge by relocating these helpers out of src/host/.
Changes:
- Added
src/uv/proxy.hcontainingclose_ptr,proxy_ptr, andwith_uv_handlein the existingasynchostnamespace. - Updated internal includes (eg,
src/http/curl.handsrc/host/*.h) to include"uv/proxy.h"instead of the deleted host header. - Extended
scripts/source-dependencies.jsonto define the new"uv": []acyclic component.
Custom instructions used:
- None (no repository instruction files from
.github/copilot-instructions.mdor.github/instructions/were loaded via tools during this review).
File summaries
| File | Description |
|---|---|
| src/uv/proxy.h | New internal header hosting libuv ownership/closure helpers. |
| src/http/curl.h | Switched include to the new uv header to avoid http -> host. |
| src/host/udp.h | Switched include to uv/proxy.h. |
| src/host/timer.h | Switched include to uv/proxy.h. |
| src/host/tcp.h | Switched include to uv/proxy.h. |
| src/host/signal.h | Switched include to uv/proxy.h. |
| src/host/every_io.h | Switched include to uv/proxy.h. |
| src/host/before_io.h | Switched include to uv/proxy.h. |
| src/host/after_io.h | Switched include to uv/proxy.h. |
| scripts/source-dependencies.json | Added the new uv component with no allowed internal deps. |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Splitting namespaces across "components" (folders) is going to get really confusing. host contained both all of ccf::asynchost, and bits of the actual cchost top-level entry point. I think if we're moving this, we need to re-namespace it as well. And we should consider moving more of the UV lifetime wrapper stuff (all the headers currently under host/ which this PR touches), if we're creating a distinct "UV wrapper" component, and its safe (dependency-wise) to do so.
Member
Author
There was a problem hiding this comment.
Agreed, let's re-namespace? I'm deliberately keeping the changes to a minimum as I break the cycles, because expanding the scope has a tendency to make things harder, but happy to try.
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-bookish-memory
branch
from
September 4, 2026 13:46
480452a to
2127e64
Compare
Amaury Chamayou (achamayou)
added a commit
that referenced
this pull request
Sep 4, 2026
…e/node cycles Relocate src/http/http_session.h, http2_session.h, and http_rpc_context.h into src/enclave/, preserving their http:: namespace and type names. These are internal runtime adapters that bind the HTTP protocol layer to enclave session/RPC machinery and ccf::RpcContextImpl (in src/node), so they belonged with their consumers rather than with the protocol-only http component. Update all in-tree consumers (rpc_sessions.h, forwarder.h, jwt_key_auto_refresh.h) to the new paths, and drop the now-unused http_rpc_context.h include from frontend.h. Make the moved headers self-contained by including enclave/rpc_handler.h and enclave/rpc_map.h directly, and updating relative http/ includes to explicit paths. Clean incidental http -> enclave edges from the remaining protocol headers: http2_parser.h and http_proc.h no longer include enclave/session.h and enclave/tls_session.h respectively (unused), and http_parser.h drops enclave/tls_session.h in favour of a direct ds/internal_logger.h include for the logging macros it actually uses. Extend scripts/source-dependencies.json with the http component's final allowed dependencies (ccf-api, crypto, ds, uv), matching the acyclic policy already enforced for other components. The dependency checker confirms no remaining http -> enclave, http -> host, or http -> node edges. Layer 2 of the acyclic source components stack, based on #8281. Refs #3517. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-bookish-memory
branch
from
September 4, 2026 15:58
db58a76 to
16eca4c
Compare
Move close_ptr, proxy_ptr, and with_uv_handle from src/host/proxy.h to a new internal header src/uv/proxy.h, preserving the asynchost namespace and behavior. This removes the direct source dependency http -> host caused by src/http/curl.h including src/host/proxy.h. Update all in-tree includes to reference uv/proxy.h directly, and add the new uv source component to scripts/source-dependencies.json with no allowed internal CCF dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-bookish-memory
branch
from
September 4, 2026 16:39
16eca4c to
5a899b0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Layer 1 of a native PR stack
This is layer 1 of a stack of PRs working towards #3517 (acyclic source component dependencies). It is based on the branch of #8278 (
achamayou-fictional-couscous) and should be reviewed/merged in that order.Related to #3517 (does not close it — later layers continue this work).
Change
Eliminates the direct source dependency
http -> host, caused bysrc/http/curl.hincludingsrc/host/proxy.h, by extracting the generic libuv ownership primitives into a new acyclic source component:close_ptr,proxy_ptr, andwith_uv_handlefromsrc/host/proxy.hto a new internal headersrc/uv/proxy.h, preserving theirasynchostnamespace and behavior unchanged.src/host/proxy.h(internal header, no compatibility shim left behind).src/http/curl.hand thesrc/host/*.hfiles that previously included"proxy.h") to include"uv/proxy.h"directly.uvsource component toscripts/source-dependencies.jsonwith no allowed internal CCF dependencies ("uv": []). Thehttppolicy is intentionally not added yet, since later stack layers still need to remove its enclave/node edges.No public API, behavior, or namespace changes; no compatibility forwarding header.
Verification
scripts/check-source-dependencies.py: passes, reports no violations; thehttp -> hostedge is gone and the moved code's edges now point only touv.ninjain a freshRelWithDebInfobuild dir): succeeds with no errors.node_connections_test: 2/2 test cases pass.curl_test: 9/15 test cases pass; the 6 failures are pre-existing (verified by checking out the base branch and rebuilding) and unrelated to this change — they fail because the test environment cannot make real outbound HTTP connections.scripts/ci-checks.sh -f: all checks pass (C/C++ format, includes, copyright, ASCII, JSON/prettier, CI test buckets, etc).