support sse#745
Merged
Merged
Conversation
qicosmos
commented
Apr 26, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
include/cinatra/coro_http_client.hpp:1308async_request_chunked里 lambda 返回read_result{{content.data(), content.size()}, true, {}}，依赖捕获的content生命周期。若底层发送不是“立即消费完该 span”，可能悬空；建议确认async_upload_impl不会跨挂起保存该 buffer，或改成持久缓冲/拷贝。:1371-1377out_buf_ = {out_buf.data(), out_buf.size()};对BodyTarget仅靠if constexpr分支约束，非 SSE 路径默认要求有data/size，模板错误信息会很差；建议加 concept/static_assert 限定。:1449-1460SSE 将operation_canceled视为正常结束过于激进，可能吞掉用户主动取消/连接异常，建议只对“handler 返回 false 导致的本地取消”做特殊处理。:1490async_sse_request(Handler &handler)只接左值，async_get_sse(..., [](…){})这种右值 lambda 在async_get_sse内转发虽可工作，但经&handler持有地址，强依赖协程执行期间该局部参数存活；建议按值存 handler 或完美转发后 decay 保存。example/main.cpp:160/README示例里sleep_for(200ms)等待服务启动不稳定，CI/慢机下可能偶现失败；建议使用显式就绪同步。.gitignore:9-10忽略*.txt风险很大，可能误伤文档/测试数据，建议删除或收窄到特定目录。- 风格：
use_sse的 handler 未使用req，建议去掉名称或标[[maybe_unused]]。整体接口设计不错，但需重点确认 SSE/chunked 的 buffer 生命周期与取消语义。
qicosmos
commented
Apr 26, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
-
正确性：
include/cinatra/coro_http_client.hpp:1371-1377
async_request将BodyTarget泛化后，非 SSE 分支直接调用out_buf.data()/size()。这要求BodyTarget必须类 span 接口，但模板未约束，误传其他类型会产生晦涩编译错误。建议加 concept/SFINAE 约束，或拆成独立重载。 -
正确性/生命周期：
coro_http_client.hpp:1311-1321
async_request_chunked里 lambda 返回read_result{{content.data(), content.size()}, ...}，依赖捕获的content生命周期。当前看起来在协程闭包内是安全的，但若后续async_upload_impl改为缓存该 span 或异步跨帧使用，容易悬空。建议显式文档说明或返回拥有数据的 buffer。 -
正确性：
coro_http_client.hpp:1458-1464
SSE 场景将operation_canceled视为正常结束并清空错误码，语义较危险：若取消来自用户主动中断/超时竞争，也会被吞掉，掩盖真实异常。建议仅对“handler 返回 false 导致的主动停止”做专门状态区分。 -
线程/资源管理：
example/main.cpp:163-196
use_sse()内部新建coro_http_server server(1, 9001);，示例整体已多处复用 9001 端口；若前一个示例 server 未完全释放，可能端口冲突。建议复用同一 server 或显式 stop/join。 -
性能：
example/main.cpp:186-188
SSE 回调中events.push_back(event)每次拷贝sse_event，若 data 较大可考虑emplace_back(event)（收益有限）或在接口层支持移动。 -
代码风格：
README.md与example/main.cpp
多处使用std::this_thread::sleep_for(200ms)等待 server 启动，不稳定且属于 flaky 示例。建议改为监听成功后的同步机制。 -
仓库配置：
.gitignore:9-10
新增*.txt过于宽泛，可能误忽略文档、测试输入、基准结果等应提交文件；建议限定具体路径或文件名。
|
Code Coverage Report |
|
Code Coverage Report |
|
Code Coverage Report |
|
Code Coverage Report |
|
Code Coverage Report |
qicosmos
commented
Apr 27, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
-
正确性：
include/cinatra/coro_http_client.hpp:1460附近，SSE 分支把operation_canceled视为正常结束并清空错误码，这会掩盖真实取消来源；若上层主动取消请求，可能被误判为正常 EOF。建议区分“解析器主动终止”和外部取消。 -
正确性/接口设计：
async_get_sse/async_post_sse（607, 623 行附近）Handler handler按值传入，而async_sse_request又持有Handler&/指针包装，若 handler 是重对象或不可拷贝 lambda 会有问题。建议改为转发引用Handler&&并用std::forward。 -
安全性：
make_body_target（81-110 行附近）对普通out_buf直接取data()/size()，缺少约束；若传入非连续容器会编译期报错但错误信息差。建议加 concept/static_assert 限制为 contiguous buffer。 -
性能：
async_request_chunked（1317 行附近）把整段std::string content一次性作为单个 chunk 发送，这只是“使用 chunked 编码”，并不能流式降低峰值内存；README 描述易让人误解。建议注明仅编码方式变化。 -
代码风格：
example/main.cpp:165、README 示例里req未使用，建议去掉参数名或标记[[maybe_unused]]。 -
代码风格/仓库配置：
.gitignore忽略*.txt（第9行）风险很大，可能误伤文档、测试数据和基准结果文件，通常不应全局忽略。 -
可维护性：
example/main.cpp:608在main中启动多个固定端口9001的用例，新增use_sse()后更易产生端口复用/时序问题，尤其依赖sleep_for(200ms)不稳定。建议动态端口或统一复用 server。
qicosmos
commented
Apr 27, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
include/cinatra/coro_http_client.hpp:1469+：SSE 分支把operation_canceled当作正常结束并清空错误码，这会掩盖真实取消/中断，可能误报成功。建议区分“回调主动停止”和“socket/上层取消”。:1490+ async_sse_request：直接改req_headers_["Accept"]，这是 client 成员状态，若同一 client 复用发普通请求，可能泄漏到后续请求；并发下也有竞态。建议只改局部headers。:1317+ async_request_chunked：lambda 持有content后返回read_result{{content.data(), content.size()}, ...}，依赖异步上传实现立即消费该 span；若框架延后发送会悬空。建议明确复制/持久化策略。:1284+ async_request：新增BodyTarget&& out_buf = {}可读性较差，且默认{}对泛型推导不友好，建议保留std::span<char>重载，SSE 单独接口走专用实现。example/main.cpp:161+ use_sse：每个示例都新建server(1, 9001)，main串行执行但未见显式 stop，存在端口占用/启动失败风险。README/example：sleep_for(200ms)等待服务启动不稳定，测试易 flaky，建议用同步信号。.gitignore:9：忽略*.txt过宽，可能误伤文档/测试数据，不建议。
|
Code Coverage Report |
qicosmos
commented
Apr 27, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
-
include/cinatra/coro_http_client.hpp:81-106：make_body_target对非 SSE 分支直接调用out_buf.data()/size()，模板约束过宽；若调用方传入非连续容器/临时对象会直接编译失败或悬垂，建议加requires/static_assert限制为 contiguous buffer。 -
coro_http_client.hpp:607-634：async_get_sse/async_post_sse的Handler handler按值传递，而async_sse_request(..., Handler &handler)内部取地址保存。若传入临时 lambda，虽然当前协程链通常立即消费，但存在生命周期依赖，建议改为转发引用并在内部显式std::decay_t持有。 -
coro_http_client.hpp:1317-1333：async_request_chunked中read_result{{content.data(), content.size()}, ...}引用 lambda 捕获的content内存；依赖上传实现不在协程挂起后延迟使用该 span。建议确认async_upload_impl不缓存该指针，否则需复制或保持稳定存储。 -
coro_http_client.hpp:1391-1485：SSE 模式下把operation_canceled强行清空为成功，这会掩盖真实取消/关闭错误，影响正确性和排障。建议仅在“handler 返回 false 的主动停止”场景转换。 -
coro_http_client.hpp:1488+：async_sse_request修改req_headers_["Accept"]，这是客户端成员状态，可能污染后续非 SSE 请求；应优先只改本次headers，避免隐藏副作用。 -
example/main.cpp:159-198：示例中启动server(1, 9001)后仅sleep_for(200ms)等待，测试不稳定且端口固定，易与前面示例冲突。建议使用随机端口/就绪同步。 -
.gitignore:8-9：新增*.txt过于宽泛，可能误伤文档、测试数据，不建议全局忽略。
qicosmos
commented
Apr 27, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
-
include/cinatra/coro_http_client.hpp:1317
async_request_chunked的source返回read_result{{content.data(), content.size()}, ...}，这里引用的是 lambda 内部捕获的content缓冲区。若底层上传逻辑异步保存该 span 而非立即消费，可能悬空。建议确认async_upload_impl的消费时机，必要时改为持久缓冲或分块拷贝。 -
include/cinatra/coro_http_client.hpp:1369-1479
async_request拆成async_request_impl后，SSE 分支通过模板区分handle_read重载，复杂度明显上升。建议加static_assert或 concept 约束BodyTarget，否则错误类型传入时编译报错可能很差。 -
include/cinatra/coro_http_client.hpp:1391-1397
SSE 分支直接将out_buf_置空，意味着复用同一个 client 发起并发请求时仍依赖成员状态切换，线程安全/重入性风险未见改善。若coro_http_client允许并发使用，这里可能有竞态。 -
include/cinatra/coro_http_client.hpp:1470-1478
将operation_canceled在 SSE 下静默转成功过于激进，可能掩盖真实取消/断连错误。建议仅在“用户 handler 主动停止”这一可判定场景下转换。 -
include/cinatra/coro_http_client.hpp:148x
async_sse_request修改req_headers_["Accept"]，会污染 client 全局默认头，影响后续非 SSE 请求。更合理的是只改本次headers副本。 -
example/main.cpp:169-205
示例里server.async_start(); sleep_for(200ms);依赖固定等待，测试不稳定且拖慢执行。建议使用端口就绪同步。 -
example/main.cpp:173
handler 未使用req，建议去掉变量名或标[[maybe_unused]]，减少告警。 -
.gitignore:9-10
新增*.txt过宽，可能误忽略文档、测试数据和基准输出，不建议。
|
Code Coverage Report |
qicosmos
commented
Apr 27, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
-
include/cinatra/coro_http_client.hpp:81-106：make_body_target对非 SSE 分支直接用out_buf.data()/size()，要求调用方类型满足这两个接口；默认参数BodyTarget&& out_buf = {}对某些类型推导/编译可能不稳定，建议加 concept/static_assert 限制，避免模板报错难排查。 -
:607-636：async_get_sse/async_post_sse中Handler handler按值接收，后续async_sse_request(..., handler, ...)传引用。若用户传入重状态/不可拷贝 handler，会有额外拷贝或直接编译失败；建议改为转发引用。 -
:1317-1334：async_request_chunked的 lambda 返回read_result{{content.data(), content.size()}, true, {}}，依赖content在异步上传期间始终存活。当前捕获值通常安全，但若底层延迟消费 span，仍有悬垂风险；最好明确read_result是否复制数据，或改为拥有型缓冲。 -
:1369-1479：async_request_impl为 SSE 特判operation_canceled并清空错误，这会吞掉真实取消原因，可能掩盖主动取消/连接异常。建议仅在“handler 返回停止”这一明确路径映射为成功结束。 -
:1490+：async_sse_request直接修改req_headers_["Accept"]。req_headers_是 client 成员，可能污染后续非 SSE 请求；建议只改本次headers。 -
example/main.cpp:157-196：示例里server.async_start(); sleep_for(200ms);是脆弱同步方式，CI/慢机可能偶发失败。建议提供启动就绪信号。 -
.gitignore:6-9：新增*.txt过宽，可能误忽略文档、测试输入和基准结果文件，不建议。 -
风格：
README/example中 SSE 示例未校验end_sse()返回值；错误处理不完整。
|
Code Coverage Report |
qicosmos
commented
Apr 27, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
-
正确性：
include/cinatra/coro_http_client.hpp:607-623，async_get_sse首次调用async_sse_request(..., handler, headers)时headers按值传递，未std::move；重定向分支第二次请求才move(headers)。功能上没错，但两次请求 header 处理路径不一致，建议统一，避免后续维护出错。 -
正确性/生命周期：
include/cinatra/coro_http_client.hpp:81-104与1391-1479，SSE 通过sse_event_handler{&handler}传裸指针。若底层异步读取在协程恢复链之外延迟执行，存在 handler 悬空风险。建议文档明确 handler 生命周期要求，或改为值/shared_ptr持有。 -
安全性/状态污染：
include/cinatra/coro_http_client.hpp:1490-1500，async_sse_request在headers.empty()时直接写req_headers_["Accept"]。这会污染 client 全局状态，影响后续非 SSE 请求。应只修改本次请求 headers，避免跨请求副作用。 -
正确性：
include/cinatra/coro_http_client.hpp:1469-1479，将 SSE 下的operation_canceled视为正常结束并清空错误码，语义较危险：真实取消和“用户主动停止读取”被混淆，可能掩盖上层取消逻辑。建议区分用户回调主动停止与外部取消。 -
性能：
include/cinatra/coro_http_client.hpp:1317-1334，async_request_chunked对字符串 chunked 上传只发送单个 chunk，实际上比普通 Content-Length 请求多了 chunked 编码开销，收益有限。若只是一次性字符串，建议文档强调这是“协议兼容需求”而非性能优化。 -
代码风格：
example/main.cpp:168-210，示例里req未使用，建议去掉参数名或标记[[maybe_unused]]。 -
仓库配置：
.gitignore:9-10忽略*.txt过宽，可能误伤测试数据/文档产物，建议限定到特定目录或临时文件命名。
|
Code Coverage Report |
qicosmos
commented
Apr 28, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
.gitignore：新增*.txt（约第9行）风险很大，会误忽略文档、测试输入/输出、配置模板等文本文件。建议改为更具体的路径或文件名模式。README/example：标题是“support sse”，但示例里chunked request也改了，PR 标题与变更范围不符。coro_http_client.hpp:81-106：make_body_target对非 SSE 分支直接用out_buf.data()/size()，若传入类型不满足 contiguous buffer 约束会模板报错，建议加 concept/static_assert 限制。async_get_sse/async_post_sse（约607+）：Handler handler按值传递，后续async_sse_request(..., Handler& handler, ...)取引用。若回调不可拷贝/持有状态，可能有额外拷贝和语义偏差；建议完美转发。async_request_chunked（约1317+）：lambda 捕获整个content再返回string_view风格数据，当前协程流程下大概率安全，但强依赖async_upload_impl不延迟保存该指针；建议明确文档或改为拥有型 buffer，避免悬垂风险。async_request/async_request_impl（约1369+）：为支持 SSE 拆成两层是对的，但out_buf_是成员状态，client 若并发复用可能互相覆盖；新增 SSE 路径也继承了这个线程安全问题，建议明确“单请求复用”限制或做请求级隔离。async_request_impl（约1469+）：把operation_canceled在 SSE 路径下吞掉并转成成功，语义较危险，可能掩盖真实取消/连接异常。至少应区分“用户主动停止消费”和“底层 I/O 取消”。- 示例
use_sse()：固定sleep_for(200ms)等服务启动不可靠，CI/慢机下易 flaky。建议使用端口就绪检测。 - 代码风格：
coro_http_request& req未使用（example/README），建议去掉变量名或标记[[maybe_unused]]。
整体看功能方向合理，但需要重点确认 SSE 取消语义 和 chunked 字符串上传的生命周期安全。
|
Code Coverage Report |
qicosmos
commented
Apr 28, 2026
qicosmos
left a comment
qicosmos
left a comment
Owner
Author
There was a problem hiding this comment.
整体方向可以，但有几处风险：
- 正确性
include/cinatra/coro_http_client.hpp:1460+：SSE 分支把operation_canceled视为正常结束并清空错误码，这会掩盖真实取消/连接异常，建议只对“用户回调主动停止”使用专用状态区分。include/cinatra/coro_http_client.hpp:1317+：async_request_chunked的 lambda 返回content.data()，依赖content捕获存活到上传完成；当前看似安全，但若底层异步缓存该 span 超过本次co_return生命周期会悬空，建议明确复制或文档约束。example/main.cpp:170：SSE server 启动后sleep_for(200ms)等待监听，不稳定，CI/慢机上可能偶现失败，建议用就绪同步。
- 性能
include/...:1284+：async_request新增BodyTarget后，SSE/非 SSE 走async_request_impl，模板分支增多，可接受；但headers仍按值传递，多次重定向/转发会额外拷贝，建议统一右值转发。example/main.cpp:190：SSE 回调里events.push_back(event)可能多次拷贝，若事件大可考虑emplace_back/move。
- 安全性
README.md与example/main.cpp的 SSE 示例未体现客户端断开后的资源清理/循环发送场景，真实使用中需避免长连接无限占用。.gitignore:8：新增*.txt过于宽泛，可能误忽略测试数据、文档或基准输出文件。
- 代码风格
example/main.cpp:161：req未使用，建议去掉参数名或标记[[maybe_unused]]。README/示例里 SSE 和 chunked API 命名清晰，但建议补充async_post_sse与重定向行为说明。
另外，diff 被截断，async_sse_request 后半段和 handle_read 改动未展示，SSE 解析正确性暂无法完整确认。
|
Code Coverage Report |
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
1 participant
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.
No description provided.