fix(streaming): move connection start off UI thread#49
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 40 minutes and 15 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough在 C++ 原生层引入连接启动状态机与并发控制，新增 Changes异步连接启动全栈实现
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
nativelib/src/main/cpp/moonlight_bridge.cpp (2)
389-415: 🧹 Nitpick | 🔵 Trivial | 💤 Low value线程安全依赖上层协调。
StartConnectionWithPreparedInfo写入全局变量g_streamConfig和g_videoCapabilities。异步版本从工作线程调用此函数，同步版本从 UI 线程调用。线程安全依赖 ArkTS 层的isStartingConnection状态跟踪来防止并发调用。当前设计是可行的，但建议在函数注释中说明此约束，避免后续维护时直接从原生层调用导致竞态。
Also applies to: 475-479
🤖 Prompt for 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. In `@nativelib/src/main/cpp/moonlight_bridge.cpp` around lines 389 - 415, The function `StartConnectionWithPreparedInfo` modifies global variables `g_streamConfig` and `g_videoCapabilities` which are accessed from multiple threads (worker thread for async calls and UI thread for sync calls). Thread safety currently depends on ArkTS layer coordination using `isStartingConnection` state tracking to prevent concurrent calls. Add documentation comments to the `StartConnectionWithPreparedInfo` function that clearly explain this thread safety constraint and that concurrent calls must be prevented at the ArkTS layer to avoid race conditions, helping future maintainers understand why the function cannot be safely called directly from multiple threads without proper synchronization.
364-373: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAES Key/IV 验证失败时仍继续执行连接流程。
当
aesKeyData或aesIvData长度不足时，仅记录错误日志但继续使用可能未初始化的密钥/IV 数据。这可能导致连接使用无效的加密参数。建议在关键加密参数无效时返回
false，让调用方显式处理错误。💡 建议的修改
if (aesKeyData && aesKeyLength >= 16) { memcpy(streamConfig->remoteInputAesKey, aesKeyData, 16); } else { OH_LOG_ERROR(LOG_APP, " riKey: INVALID (data=%{public}p, len=%{public}zu)", aesKeyData, aesKeyLength); + return false; } if (aesIvData && aesIvLength >= 16) { memcpy(streamConfig->remoteInputAesIv, aesIvData, 16); } else { OH_LOG_ERROR(LOG_APP, " riIv: INVALID (data=%{public}p, len=%{public}zu)", aesIvData, aesIvLength); + FreeServerInfo(serverInfo); + return false; }🤖 Prompt for 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. In `@nativelib/src/main/cpp/moonlight_bridge.cpp` around lines 364 - 373, The validation checks for aesKeyData and aesIvData only log errors in the else blocks but allow execution to continue with potentially invalid encryption parameters. When the condition aesKeyLength < 16 or aesIvLength < 16 is detected in the else blocks following the memcpy calls for streamConfig->remoteInputAesKey and streamConfig->remoteInputAesIv, the function should return false to halt the connection process instead of merely logging the error. This ensures the caller can explicitly handle the failure rather than proceeding with uninitialized encryption data.entry/src/main/ets/service/streaming/StreamingSession.ets (1)
1529-1539: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuecleanup 对启动中断处理正确，但建议添加防御性检查。
当前逻辑：先取
connectionStartPromise引用，再调用interruptConnection()，最后等待。这正确处理了 Promise 可能在检查和使用之间被清空的情况。但
interruptConnection()调用应在pendingStart !== null检查内部，避免不必要的中断调用。💡 建议的优化
if (this.isStartingConnection) { const pendingStart = this.connectionStartPromise; - this.nativeModule.interruptConnection(); if (pendingStart !== null) { + this.nativeModule.interruptConnection(); try { await pendingStart; } catch (err) { console.warn(`等待启动中断完成失败: ${err}`); } } }🤖 Prompt for 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. In `@entry/src/main/ets/service/streaming/StreamingSession.ets` around lines 1529 - 1539, The `interruptConnection()` call on `this.nativeModule` is being executed unconditionally outside the `pendingStart !== null` check, which can trigger unnecessary interrupt operations. Refactor the code so that `interruptConnection()` is only called when `pendingStart` is confirmed to be not null, by moving the call inside the `if (pendingStart !== null)` conditional block or adding an additional check before calling it.
🤖 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.
Nitpick comments:
In `@entry/src/main/ets/service/streaming/StreamingSession.ets`:
- Around line 1529-1539: The `interruptConnection()` call on `this.nativeModule`
is being executed unconditionally outside the `pendingStart !== null` check,
which can trigger unnecessary interrupt operations. Refactor the code so that
`interruptConnection()` is only called when `pendingStart` is confirmed to be
not null, by moving the call inside the `if (pendingStart !== null)` conditional
block or adding an additional check before calling it.
In `@nativelib/src/main/cpp/moonlight_bridge.cpp`:
- Around line 389-415: The function `StartConnectionWithPreparedInfo` modifies
global variables `g_streamConfig` and `g_videoCapabilities` which are accessed
from multiple threads (worker thread for async calls and UI thread for sync
calls). Thread safety currently depends on ArkTS layer coordination using
`isStartingConnection` state tracking to prevent concurrent calls. Add
documentation comments to the `StartConnectionWithPreparedInfo` function that
clearly explain this thread safety constraint and that concurrent calls must be
prevented at the ArkTS layer to avoid race conditions, helping future
maintainers understand why the function cannot be safely called directly from
multiple threads without proper synchronization.
- Around line 364-373: The validation checks for aesKeyData and aesIvData only
log errors in the else blocks but allow execution to continue with potentially
invalid encryption parameters. When the condition aesKeyLength < 16 or
aesIvLength < 16 is detected in the else blocks following the memcpy calls for
streamConfig->remoteInputAesKey and streamConfig->remoteInputAesIv, the function
should return false to halt the connection process instead of merely logging the
error. This ensures the caller can explicitly handle the failure rather than
proceeding with uninitialized encryption data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40a6c638-88c6-4ec8-a133-74660e9ca9f6
📒 Files selected for processing (5)
entry/src/main/ets/service/streaming/MoonBridge.etsentry/src/main/ets/service/streaming/StreamingSession.etsnativelib/src/main/cpp/moonlight_bridge.cppnativelib/src/main/cpp/moonlight_bridge.hnativelib/src/main/cpp/napi_init.cpp
qiin2333
force-pushed
the
codex/fix-start-connection-freeze
branch
2 times, most recently
from
1c104a0 to
f4b5927
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nativelib/src/main/cpp/moonlight_bridge.cpp (1)
656-664: 🧹 Nitpick | 🔵 Trivial | 💤 Low value参数准备失败时同时抛出异常并返回
-1，行为不一致。当
PrepareStartConnection失败时，代码调用napi_throw_error抛出异常，但随后仍创建并返回一个值为-1的napi_value。在 N-API 中，抛出异常后应直接返回nullptr让运行时处理异常，而不是返回正常值。同步版本（line 589-592）有相同问题。建议统一处理方式。
🔧 建议修复
if (!PrepareStartConnection(env, args, argc, &context->serverInfo, &context->streamConfig, &videoCapabilities, &hdrMode)) { MarkConnectionIdle(); delete context; napi_throw_error(env, nullptr, "参数不足"); - napi_value result; - napi_create_int32(env, -1, &result); - return result; + return nullptr; }🤖 Prompt for 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. In `@nativelib/src/main/cpp/moonlight_bridge.cpp` around lines 656 - 664, In the error handling block following the PrepareStartConnection call, after invoking napi_throw_error to throw an exception, remove the napi_create_int32 call and the return of the -1 value, and instead directly return nullptr to allow the N-API runtime to properly handle the thrown exception. Apply the same fix to the synchronous version at lines 589-592 where the same pattern exists.
🤖 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 `@nativelib/src/main/cpp/moonlight_bridge.cpp`:
- Around line 603-611: In the ExecuteStartConnectionAsync function, after the
successful call to StartConnectionWithPreparedInfo with context->serverInfo,
transfer ownership of context->serverInfo to the global g_serverInfo instead of
allowing it to be freed when the context is destroyed. This ensures the
lifecycle management is consistent with the synchronous path where g_serverInfo
is freed by DoStopConnectionCleanup, preventing use-after-free issues since
LiStartConnection may store pointers internally and continue using them on the
connection thread. Add code to copy context->serverInfo into g_serverInfo before
the context is cleaned up so that stopConnection will properly release the
serverInfo that was actually used by the connection.
---
Nitpick comments:
In `@nativelib/src/main/cpp/moonlight_bridge.cpp`:
- Around line 656-664: In the error handling block following the
PrepareStartConnection call, after invoking napi_throw_error to throw an
exception, remove the napi_create_int32 call and the return of the -1 value, and
instead directly return nullptr to allow the N-API runtime to properly handle
the thrown exception. Apply the same fix to the synchronous version at lines
589-592 where the same pattern exists.
🪄 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: bb98a7e0-971b-4100-abc4-00d0355a152d
📒 Files selected for processing (6)
entry/src/main/ets/service/streaming/MoonBridge.etsentry/src/main/ets/service/streaming/StreamingSession.etsnativelib/src/main/cpp/callbacks.cppnativelib/src/main/cpp/moonlight_bridge.cppnativelib/src/main/cpp/moonlight_bridge.hnativelib/src/main/cpp/napi_init.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- entry/src/main/ets/service/streaming/MoonBridge.ets
- nativelib/src/main/cpp/napi_init.cpp
- entry/src/main/ets/service/streaming/StreamingSession.ets
qiin2333
force-pushed
the
codex/fix-start-connection-freeze
branch
from
f4b5927 to
e3ae584
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
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.
改了啥呀
startConnectionAsync，把LiStartConnection丢到 N-API async work 里跑，别再让主线程陪 ENet 等网络啦，冻屏小杂鱼退散。StreamingSession改成等待异步启动，并记录 pending start 状态。interruptConnection()，等启动返回后再stopConnection()，贴合 moonlight-common-c 里LiStartConnection/LiStopConnection非线程安全的约束。为啥要改
冻屏日志里的主线程栈卡在
MoonBridge_StartConnection -> LiStartConnection -> startControlStream -> enet_socket_wait/poll，属于同步 native 网络等待把 ArkTS/UI 线程堵住了。把连接启动移出主线程后，即使控制流连接等待或超时，也不会触发THREAD_BLOCK_6S。验证
git diff --check -- nativelib/src/main/cpp/moonlight_bridge.cpp nativelib/src/main/cpp/moonlight_bridge.h nativelib/src/main/cpp/napi_init.cpp entry/src/main/ets/service/streaming/StreamingSession.ets entry/src/main/ets/service/streaming/MoonBridge.ets/Applications/DevEco-Studio.app/Contents/sdk/default/openharmony/native/build-tools/cmake/bin/ninja -C nativelib/.cxx/default/default/debug/arm64-v8a moonlight_nativelib/Applications/DevEco-Studio.app/Contents/sdk/default/openharmony/native/build-tools/cmake/bin/ninja -C nativelib/.cxx/default/default/debug/x86_64 moonlight_nativelibJAVA_HOME=/Applications/DevEco-Studio.app/Contents/jbr/Contents/Home PATH=/Applications/DevEco-Studio.app/Contents/jbr/Contents/Home/bin:$PATH NODE_PATH=/Users/mac/Program/moonlight-harmony/node_modules:/Applications/DevEco-Studio.app/Contents/tools/hvigor/hvigor/node_modules:/Applications/DevEco-Studio.app/Contents/tools/hvigor/hvigor-ohos-plugin/node_modules DEVECO_SDK_HOME=/Users/mac/ohos-sdk-cache/6.1-Release-mac/sdk-ci-shape node hvigorw.js assembleApp --mode project -p product=default --no-daemon --stacktraceSummary by CodeRabbit
发布说明
新功能
startConnectionAsync），以 Promise 方式返回启动结果，提升流媒体初始化期间的应用响应性。改进