Support HTTP/2#748
Conversation
qicosmos
left a comment
There was a problem hiding this comment.
以下为基于可见 diff 的审查意见:
-
不应提交本地配置
.claude/settings.local.json新增文件:这是本地 AI/工具配置,不应进入仓库。建议加入.gitignore并从 PR 移除。
-
CMake OpenSSL 兼容性风险
cmake/find_openssl.cmake:14改为find_package(OpenSSL QUIET)后,原先仅检测ssl库的逻辑被替换。若下游仍依赖${OPENSSL_LIBRARIES},可能破坏兼容。- 建议统一全项目使用
OpenSSL::SSL/OpenSSL::Crypto,或保留旧变量兼容。
-
平台链接问题
example/CMakeLists.txt:15-22无条件链接Threads::Threads,需确认顶层已find_package(Threads REQUIRED),否则部分构建会失败。- 原先 Linux 下链接
pthread -ldl被移除,若 OpenSSL/运行时仍需dl,可能导致链接失败。
-
未启用 SSL 时仍构建 HTTP/2 示例
example/CMakeLists.txt:15-22无条件构建http2_example/http2_conformance_server,但源码在未启用 SSL 时仅打印错误退出。- 更合理做法是在
ENABLE_SSL时才构建,避免产生不可用示例目标。
-
参数解析不严谨
http2_conformance_server.cpp:15-34使用strtoul/strtol未检查endptr和溢出。输入如"123abc"会被接受。- 建议使用
std::from_chars或完整校验。
-
潜在资源/异常处理问题
http2_conformance_server.cpp:88init_ssl、async_start未检查失败或捕获异常,证书路径错误时可能直接崩溃。- 示例程序也建议输出明确错误。
-
示例存在竞态/脆弱等待
http2_example.cpp:35使用sleep_for(50ms)等待服务启动不可靠。应依赖async_start返回状态或确认监听完成后再连接。
-
代码风格
http2_example.cpp截断处可见空白行含多余空格,建议清理。- README 示例较长,建议确保与实际 API 完全同步。
|
Code Coverage Report |
qicosmos
left a comment
There was a problem hiding this comment.
代码审查意见:
-
.claude/settings.local.json(新文件 1-8):不应提交本地 AI 工具配置,属于个人环境文件,建议加入.gitignore。 -
cmake/find_openssl.cmake(约 14 行):从check_library_exists改为find_package(OpenSSL QUIET)后,需确认顶层 CMake 已正确使用OpenSSL::SSL/Crypto,否则旧变量OPENSSL_LIBRARIES依赖处可能失效。 -
example/CMakeLists.txt(约 15-24 行):无条件构建http2_example/http2_conformance_server,但源码依赖 SSL。未启用 SSL 时虽然运行时报错,但仍可能因头文件/宏依赖导致编译失败。建议放入if(ENABLE_SSL)。 -
http2_conformance_server.cpp:- 14/22/30 行:
strtoul/strtol未检查endptr和errno,如"123abc"会被接受,建议严格校验。 - 30 行:线程数未限制上界,极大值可能导致资源耗尽。
- 70 行:
set_enable_http2_connect_protocol(true)默认开启 CONNECT,测试用途可以,但示例服务端需明确安全风险。 - 91 行:证书路径硬编码,工作目录变化会失败,建议参数化。
- 14/22/30 行:
-
http2_example.cpp:- 35 行:
sleep_for(50ms)等待服务启动不可靠,存在竞态;应使用启动完成回调或检查端口监听状态。 - 多处错误退出路径手动 stop/join,异常时可能泄漏线程,建议用 RAII/scope_guard。
- 35 行:
-
README:声明“服务端统一支持 HTTP/2”需明确仅 TLS+ALPN,否则容易误导用户。
qicosmos
left a comment
There was a problem hiding this comment.
代码审查要点:
-
不应提交本地配置
.claude/settings.local.json:1是个人/工具本地配置,建议移除并加入.gitignore,避免污染仓库。 -
OpenSSL 检测行为变化风险
cmake/find_openssl.cmake:14改为find_package(OpenSSL QUIET),但未处理旧变量兼容。其他地方若仍依赖OPENSSL_LIBRARIES可能失效。建议统一改为OpenSSL::SSL/OpenSSL::Crypto,或保留兼容变量。 -
CMake 链接平台兼容性
example/CMakeLists.txt:14-22新增目标始终构建,但 HTTP/2 示例依赖 SSL。即使源码里运行时返回,编译/链接仍可能因头文件或 SSL 符号失败。建议在ENABLE_SSL时才添加http2_example/http2_conformance_server,或明确保证无 SSL 可编译。 -
缺少 dl / pthread 兼容处理
example/CMakeLists.txt:31删除了pthread -ldl,只链接 OpenSSL target。Linux 下部分 OpenSSL/静态链接场景可能仍需dl。建议使用Threads::Threads并按平台链接${CMAKE_DL_LIBS}。 -
参数解析不严谨
example/http2_conformance_server.cpp:15、:25、:34使用strtoul/strtol未检查endptr,"123abc"会被接受。建议校验完整字符串并检查溢出。 -
线程数未限制
http2_conformance_server.cpp:34只检查非 0,极大值会导致资源耗尽。建议设置合理上限。 -
示例存在竞态/不稳定等待
example/http2_example.cpp:35使用sleep_for(50ms)等待服务启动不可靠。既然已调用server.port(),应依赖明确的启动完成机制或错误检查。 -
安全性说明不足
README 示例中客户端连接 HTTPS 未体现证书校验策略,若默认跳过校验,应明确说明;否则用户可能误用到生产环境。
整体建议:移除本地配置文件,完善 CMake 条件构建与平台链接,修正参数解析和示例启动同步。
|
Code Coverage Report |
qicosmos
left a comment
There was a problem hiding this comment.
审查意见:
-
.claude/settings.local.json:1
本地 AI/工具配置不应提交到仓库,建议加入.gitignore。 -
cmake/find_openssl.cmake:14
改为find_package(OpenSSL QUIET)后,依赖方需统一使用OpenSSL::SSL/Crypto。请确认全仓库没有仍依赖${OPENSSL_LIBRARIES}的目标,否则会链接失败。 -
example/CMakeLists.txt:15-23
http2_example、http2_conformance_server无条件构建,但代码依赖 HTTP/2/SSL 相关头和配置。若未启用 SSL,可能仍编译失败。建议放入if(ENABLE_SSL)或单独开关。 -
example/http2_conformance_server.cpp:15-34
strtoul/strtol未检查endptr和errno,"123abc"、溢出等会被接受。建议使用std::from_chars或完整校验。 -
example/http2_conformance_server.cpp:77-83
std::any_cast<...>(&metadata)很可能错误:metadata是optional/any包装对象时,应对内部any做 cast,例如std::any_cast<T>(&*metadata)。当前逻辑可能永远取不到 HTTP/2 metadata。 -
example/http2_example.cpp:34-35
async_start()后用sleep_for(50ms)等待服务启动不可靠,CI 慢机可能偶发失败。建议提供启动完成同步机制或重试连接。 -
安全性
示例固定使用仓库内证书和密码"test"可以接受,但 README 应明确仅用于示例,避免用户误用于生产。
|
Code Coverage Report |
|
Code Coverage Report |
qicosmos
left a comment
There was a problem hiding this comment.
主要问题:
-
cmake/find_openssl.cmake新增段落后出现单独一行i(约第20行),这是明显的语法错误,会导致 CMake 配置失败,必须删除。 -
.claude/settings.local.json不应提交到仓库。这是本地工具配置,建议加入.gitignore。 -
多处 GitHub Actions 版本看起来不存在或风险较高:
actions/checkout@v6actions/upload-artifact@v7seanmiddleditch/gha-setup-ninja@v6
请确认 marketplace 是否已有这些 tag,否则 CI 会直接失败。建议使用稳定已发布版本。
-
cmake/find_openssl.cmake从check_library_exists改为find_package(OpenSSL QUIET),需确认后续变量名是否同步改为OpenSSL::SSL/OPENSSL_FOUND,否则可能破坏旧逻辑。 -
PR 标题是 HTTP/2,但当前 diff 主要是 CI/文档/CMake,核心 HTTP/2 实现未展示,无法审查协议正确性、流控、HPACK、并发流、ALPN fallback 等关键逻辑。建议拆分 CI 升级与 HTTP/2 功能 PR。
|
Code Coverage Report |
|
Code Coverage Report |
|
Code Coverage Report |
|
Code Coverage Report |
|
Code Coverage Report |
qicosmos
left a comment
There was a problem hiding this comment.
审查意见(基于可见 diff,HTTP/2 核心代码被截断,无法完整评估协议正确性):
-
CI 配置严重问题
- 多处将
actions/checkout升级为@v6(如.github/workflows/clang-format.yml:20、linux_gcc.yml:25等),目前 GitHub 官方常用版本为v4,v6可能不存在,会直接导致 CI 失败。 actions/upload-artifact@v7(linux_llvm_cov.yml:57、mac.yml:46)同样疑似不存在,建议确认版本。seanmiddleditch/gha-setup-ninja@v6(linux_gcc.yml:64、windows.yml:24)也需确认是否存在。不要盲目升级 action major 版本。
- 多处将
-
不应提交本地工具配置
.claude/settings.local.json:1是本地 AI/开发环境配置,包含本地构建命令,不应进入仓库。建议删除并加入.gitignore。
-
CI 并行度调整可能降低覆盖
- 多处将
ctest -j \nproc`改为-j 2(如linux_gcc.yml:38、linux_clang.yml:50`),会显著拉长 CI 时间。若为稳定性考虑,应在 PR 描述中说明原因。
- 多处将
-
macOS OpenSSL 修改较好
mac.yml:29-32使用openssl@3和brew --prefix比硬编码/usr/local更兼容 Apple Silicon,合理。
-
CMake 变更需补充上下文
CMakeLists.txt新增ENABLE_SSL到CINATRA_ENABLE_SSL的兼容逻辑看起来合理,但需确认不会覆盖用户显式设置。
-
PR 标题与可见改动不匹配
- 标题是 “Support HTTP/2”,但可见部分主要是 CI 和杂项配置。HTTP/2 实现、测试、依赖、安全边界需完整 diff 才能审查。建议拆分 CI 升级与 HTTP/2 功能 PR。
|
Code Coverage Report |
qicosmos
left a comment
There was a problem hiding this comment.
以下 diff 基本都是 CI/配置变更,未看到实际 HTTP/2 实现代码,PR 标题与内容不匹配,建议补充核心代码 diff 后再审。
主要问题:
-
.claude/settings.local.json行 1-8
不应提交本地 AI 工具配置,且包含本地执行权限规则。建议加入.gitignore并从 PR 移除。 -
多处
uses: actions/checkout@v6、upload-artifact@v7、codecov-action@v6等
请确认这些版本真实存在。当前 GitHub Actions 常用稳定版本仍是checkout@v4、upload-artifact@v4;使用不存在版本会直接导致 CI 失败。 -
.github/workflows/linux_clang.yml、linux_gcc.yml中seanmiddleditch/gha-setup-ninja@v6
该 action 是否有v6tag 需确认。原先@master虽不推荐,但随意改成不存在 tag 风险更高。 -
.github/workflows/code-coverage.yml等多处将-j \nproc`改为-j 2`
会显著降低 CI 构建/测试性能。若为避免资源不足,建议加注释说明,或只在不稳定 job 上限制并使用矩阵/变量统一配置。 -
.github/workflows/linux_gcc.yml新增 Python heredoc 片段缩进疑似错误
python3 - <<'PY'后的 Python 代码应从行首开始,否则可能触发IndentationError。建议实际验证 workflow。 -
.github/workflows/linux_clang.yml/linux_gcc.yml原命令ctest ... -j 1 \nproc`被改为-j 2`
原命令本身就可疑,但本次修改改变并发语义,需确认是否会影响依赖顺序或并发敏感测试。
总结:当前 PR 缺少 HTTP/2 功能代码,且 CI action 版本升级存在高概率不可用风险。建议先移除本地配置、确认所有 action tag 有效,并补充实际 HTTP/2 实现与测试。
qicosmos
left a comment
There was a problem hiding this comment.
审查意见:
-
PR 内容不匹配
标题为“Support HTTP/2”,但 diff 几乎全是 CI/workflow 修改,未看到 HTTP/2 相关代码。建议拆分:CI 更新单独 PR,HTTP/2 功能 PR 应包含实现与测试。 -
.claude/settings.local.json不应提交(新文件第 1-8 行)
这是本地/个人工具配置,包含本地构建命令,建议加入.gitignore并从 PR 移除,避免污染仓库。 -
GitHub Action 版本可能无效
多处将actions/checkout升到v6、actions/upload-artifact@v7、codecov/codecov-action@v6、seanmiddleditch/gha-setup-ninja@v6等。请确认这些 tag 实际存在;若不存在会直接导致 CI 失败。尤其第三方 action 不应盲目升主版本。 -
使用 master/main 分支存在可复现性风险
Homebrew/actions/setup-homebrew@main(mac.yml)仍未固定版本/commit,存在供应链与不可复现风险。建议固定到稳定 tag 或 commit SHA。 -
CI 并行度硬编码为 2
多处将-j \nproc`改为-j 2`。这会显著降低 CI 性能,尤其 Release/coverage 构建。若是为降低资源占用,建议加注释或用变量统一控制。 -
linux_gcc.yml 中嵌入 Python 缩进需验证
新增Report CTest Failure的 heredoc 内 Python 代码缩进看起来与 YAML 混排,建议确认实际解析无误,并用本地act或 CI 验证。 -
mac.yml OpenSSL 处理更合理
使用brew --prefix openssl@3替代硬编码路径是改进,值得保留。
总体:当前 PR 更像 CI 维护,不建议以 HTTP/2 功能合入;需确认 action 版本有效并移除本地配置文件。
|
Code Coverage Report |
|
Code Coverage Report |
qicosmos
left a comment
There was a problem hiding this comment.
以下 diff 基本只涉及 CI 配置,未看到 HTTP/2 相关 C++ 实现,无法审查协议正确性。
主要问题:
-
新增本地配置不应提交
.claude/settings.local.json:1-8- 这是本地 AI/工具权限配置,包含本地构建命令,建议加入
.gitignore并从 PR 移除。
-
GitHub Action 版本疑似不存在
- 多处
actions/checkout@v6、actions/upload-artifact@v7、codecov/codecov-action@v6 - 截至常见稳定版本,
checkout主流为v4,upload-artifact为v4。使用不存在的版本会直接导致 CI 失败。请确认这些 tag 存在后再升级。
- 多处
-
第三方 Action 使用不存在/不稳定 tag
.github/workflows/linux_clang.yml、linux_gcc.yml、windows.ymlseanmiddleditch/gha-setup-ninja@v6可能不存在;原先master改为v6需确认。.github/workflows/mac.ymlHomebrew/actions/setup-homebrew@main仍是浮动引用,建议 pin 到明确版本或 commit SHA,避免供应链风险。
-
并行度被固定为 2,可能显著降低 CI 性能
- 多处
cmake --build ... --parallel 2、ctest -j 2 - 原先使用
nproc或默认并行,改为 2 会拖慢构建/测试。若为避免资源问题,建议只对不稳定 job 限制,或使用${{ runner.cpu }}/nproc。
- 多处
-
CTest 命令修正可疑
linux_clang.yml/linux_gcc.yml原命令ctest ... -j 1 \nproc`本身有误,本次改为-j 2` 是合理的,但需确认是否会引入并发测试竞态。
-
macOS OpenSSL 路径修正合理
.github/workflows/mac.yml- 使用
brew --prefix openssl@3比硬编码/usr/local更兼容 Apple Silicon,改动正确。
建议:先修正 Action 版本有效性,移除本地配置文件,再补充 HTTP/2 实现 diff 供审查。
qicosmos
left a comment
There was a problem hiding this comment.
审查意见(基于可见 diff):
-
阻塞:大量 GitHub Action 版本可能不存在
- 多处改为
actions/checkout@v6、actions/upload-artifact@v7、codecov/codecov-action@v6、seanmiddleditch/gha-setup-ninja@v6(如.github/workflows/*.yml多处)。 - 这些版本需确认真实存在,否则 CI 会直接失败。建议使用已发布稳定版本并固定到 tag/SHA。
- 多处改为
-
不应提交本地配置
.claude/settings.local.json第 1-8 行是本地工具权限配置,包含本地构建/执行命令。- 建议加入
.gitignore,不要进入仓库,避免污染项目配置。
-
CI 并行度被固定为 2,可能显著降低性能
- 多处将
-j \nproc`/ 默认并行改为--parallel 2、ctest -j 2`。 - 这会拖慢 CI,尤其是 Linux runner。若为稳定性考虑,建议加注释说明原因,或用环境变量控制。
- 多处将
-
workflow 变更与 PR 标题不匹配
- PR 标题是 “Support HTTP/2”,但可见 diff 几乎全是 CI 配置调整,没有 HTTP/2 实现代码。
- 建议拆分:HTTP/2 功能和 CI 维护分别提交,便于审查和回滚。
-
YAML/脚本可维护性
linux_gcc.yml新增的 Python heredoc 缩进较脆弱,建议确认实际 YAML 解析及 shell 执行无误。Report CTest Failure仅在 gcc workflow 添加,其他平台失败诊断不一致。
-
安全性
Homebrew/actions/setup-homebrew@main使用浮动分支,不利于供应链安全。建议固定版本或 SHA。
Summary
Conflict resolution
Validation