fix: preserve xconnector waits across suspend interrupts - #1033
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new native library Changes
Sequence DiagramsequenceDiagram
participant Java as XConnectorEpoll (Java)
participant JNI as XConnectorEpollNative (JNI)
participant Native as xconnectorpatch.c (C)
participant Sys as Kernel (epoll/poll/accept/close)
Java->>JNI: createAFUnixSocket(path)
JNI->>Native: Java_createAFUnixSocket
Native->>Sys: socket()/bind()/listen()
Sys-->>Native: serverFd
Native-->>JNI: serverFd
JNI-->>Java: serverFd
Java->>JNI: createEpollFd()
JNI->>Native: Java_createEpollFd
Native->>Sys: epoll_create1()
Sys-->>Native: epollFd
Native-->>JNI: epollFd
JNI-->>Java: epollFd
Java->>JNI: doEpollIndefinitely(this, epollFd, serverFd, addClientToEpoll)
JNI->>Native: Java_doEpollIndefinitely
loop until shutdown
Native->>Sys: waitForEpollEvents (epoll_wait with retry)
Sys-->>Native: events
alt new connection
Native->>Sys: accept()
Sys-->>Native: clientFd
Native->>Native: FdTracker stores clientFd
Native->>JNI: handleNewConnection callback
JNI-->>Java: onNewConnection()
else existing client readable
Native->>Native: detect readable via poll/epoll
Native->>JNI: handleExistingConnection callback
JNI-->>Java: onExistingConnection()
end
end
Native->>JNI: return status
JNI-->>Java: doEpollIndefinitely returns
Java->>JNI: closeFd(fd)
JNI->>Native: Java_closeFd
Native->>Sys: close(fd)
Sys-->>Native: closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/cpp/xconnectorpatch/xconnectorpatch.c">
<violation number="1" location="app/src/main/cpp/xconnectorpatch/xconnectorpatch.c:115">
P2: Missing null/exception check after GetStringUTFChars can lead to a native crash if it returns NULL (e.g., OOM or null jstring).</violation>
<violation number="2" location="app/src/main/cpp/xconnectorpatch/xconnectorpatch.c:116">
P2: sockaddr_un length is computed from the full path string even though sun_path is truncated, and it omits the terminating NUL. For long paths, addrLength can exceed the actual buffer/sizeof(sockaddr_un), leading to bind() failures. Compute length from the truncated path and include the NUL byte.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🧹 Nitpick comments (3)
app/src/main/cpp/xconnectorpatch/xconnectorpatch.c (2)
252-259: Performance: Repeated GetMethodID lookup on hot path.
GetObjectClassandGetMethodIDare called on every poll wake. SincewaitForSocketReadis invoked in a loop (per the Java code inhandleNewConnection), this introduces unnecessary overhead.Consider caching the
jmethodIDat the start of the method (or when the connector object is first seen):⚡ Proposed optimization
JNIEXPORT jboolean JNICALL Java_com_winlator_xconnector_XConnectorEpollNative_waitForSocketRead(JNIEnv *env, jclass clazz, jobject connector, jint clientFd, jint shutdownFd) { + // Cache method ID lookup outside hot path + static jmethodID handleExistingConnection = NULL; + if (handleExistingConnection == NULL) { + jclass connectorClass = (*env)->GetObjectClass(env, connector); + handleExistingConnection = (*env)->GetMethodID(env, connectorClass, "handleExistingConnection", "(I)V"); + if (handleExistingConnection == NULL) { + LOGD("failed to resolve handleExistingConnection callback"); + return JNI_FALSE; + } + } + struct pollfd pfds[2]; // ... poll logic ... if (pfds[0].revents & POLLIN) { - jclass connectorClass = (*env)->GetObjectClass(env, connector); - jmethodID handleExistingConnection = (*env)->GetMethodID(env, connectorClass, "handleExistingConnection", "(I)V"); - if (handleExistingConnection == NULL) { - LOGD("failed to resolve handleExistingConnection callback"); - return JNI_FALSE; - } (*env)->CallVoidMethod(env, connector, handleExistingConnection, clientFd); }Note: This assumes the same class is always passed. If different subclasses could be passed, you'd need per-class caching.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/xconnectorpatch/xconnectorpatch.c` around lines 252 - 259, GetObjectClass and GetMethodID are being called inside the hot loop of waitForSocketRead for every poll wake (resolving handleExistingConnection each time); cache the jmethodID (and optionally the jclass) when the connector object is first seen (or at start of waitForSocketRead) and reuse that cached method ID for subsequent CallVoidMethod invocations to avoid repeated lookups; adjust logic around connector/handleExistingConnection resolution so you only call GetMethodID once and fall back to resolving again if the cached class/method become invalid.
72-100: Code duplication with xconnector_epoll.c.The
waitForEpollEventsandwaitForPollEventsfunctions are nearly identical to those inapp/src/main/cpp/winlator/xconnector_epoll.c. While beingstaticavoids linker issues, this creates maintenance burden - any bug fixes must be applied to both files.Consider extracting these into a shared header or source file that both can include.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/xconnectorpatch/xconnectorpatch.c` around lines 72 - 100, Extract the duplicated blocking-retry logic into a single shared implementation: create a new source (e.g., xconnector_wait.c) containing non-static implementations of waitForEpollEvents and waitForPollEvents and a header (e.g., xconnector_wait.h) that declares those functions; remove the duplicate static definitions from xconnectorpatch.c and xconnector_epoll.c, include xconnector_wait.h in both files, and update the build files so xconnector_wait.c is compiled/linked into both targets. Ensure function signatures remain identical, preserve logging behavior (LOGD and errno handling), and remove the static keyword so there is only one canonical definition.app/src/main/cpp/xconnectorpatch/CMakeLists.txt (1)
1-7: LGTM - Minimal CMake configuration.The CMake configuration is functional. For improved code quality, consider adding compiler warning flags.
💡 Optional: Add compiler warnings
cmake_minimum_required(VERSION 3.22.1) project(XConnectorPatch C) +# Enable warnings for better code quality +target_compile_options(xconnectorpatch PRIVATE -Wall -Wextra) + add_library(xconnectorpatch SHARED xconnectorpatch.c) target_link_libraries(xconnectorpatch log)Note: The
target_compile_optionsmust come afteradd_library.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/cpp/xconnectorpatch/CMakeLists.txt` around lines 1 - 7, Add compiler warning flags to the C target after the library is created: update the CMakeLists (after add_library(xconnectorpatch ...)) to call target_compile_options(xconnectorpatch ...) for the C language and include sensible flags (e.g., -Wall -Wextra -Wpedantic and optionally -Werror) so the xconnectorpatch target is built with warnings enabled; keep the existing target_link_libraries(xconnectorpatch log) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/cpp/xconnectorpatch/xconnectorpatch.c`:
- Around line 19-70: The static fdTracking array is accessed concurrently by
trackFd, untrackFd, and closeTrackedFd and needs synchronization; add a
pthread_mutex_t (or std::mutex if C++) as a static/global (e.g.,
fdTrackingMutex), initialize it appropriately, then lock the mutex at the start
of trackFd and untrackFd and around the untrack-check in closeTrackedFd (unlock
before calling close() if you want to avoid holding the lock during the syscall,
or keep it locked if necessary), and ensure to unlock on all return paths so all
accesses to fdTracking and its fields are protected and race-free.
---
Nitpick comments:
In `@app/src/main/cpp/xconnectorpatch/CMakeLists.txt`:
- Around line 1-7: Add compiler warning flags to the C target after the library
is created: update the CMakeLists (after add_library(xconnectorpatch ...)) to
call target_compile_options(xconnectorpatch ...) for the C language and include
sensible flags (e.g., -Wall -Wextra -Wpedantic and optionally -Werror) so the
xconnectorpatch target is built with warnings enabled; keep the existing
target_link_libraries(xconnectorpatch log) intact.
In `@app/src/main/cpp/xconnectorpatch/xconnectorpatch.c`:
- Around line 252-259: GetObjectClass and GetMethodID are being called inside
the hot loop of waitForSocketRead for every poll wake (resolving
handleExistingConnection each time); cache the jmethodID (and optionally the
jclass) when the connector object is first seen (or at start of
waitForSocketRead) and reuse that cached method ID for subsequent CallVoidMethod
invocations to avoid repeated lookups; adjust logic around
connector/handleExistingConnection resolution so you only call GetMethodID once
and fall back to resolving again if the cached class/method become invalid.
- Around line 72-100: Extract the duplicated blocking-retry logic into a single
shared implementation: create a new source (e.g., xconnector_wait.c) containing
non-static implementations of waitForEpollEvents and waitForPollEvents and a
header (e.g., xconnector_wait.h) that declares those functions; remove the
duplicate static definitions from xconnectorpatch.c and xconnector_epoll.c,
include xconnector_wait.h in both files, and update the build files so
xconnector_wait.c is compiled/linked into both targets. Ensure function
signatures remain identical, preserve logging behavior (LOGD and errno
handling), and remove the static keyword so there is only one canonical
definition.
🪄 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: db1f28f6-6cb0-44dc-b1cb-d85754416a7e
📒 Files selected for processing (6)
app/build.gradle.ktsapp/src/main/cpp/winlator/xconnector_epoll.capp/src/main/cpp/xconnectorpatch/CMakeLists.txtapp/src/main/cpp/xconnectorpatch/xconnectorpatch.capp/src/main/java/com/winlator/xconnector/XConnectorEpoll.javaapp/src/main/java/com/winlator/xconnector/XConnectorEpollNative.java
|
would be good to get this in, can we confirm that it fixes the issue before i merge? |
|
If you can get me an apk I will gladly test it out :) |
|
I bult this branch myself and can confirm it works on Odin 3 😍 |
|
confirmed working on galaxy s25 ultra |
|
@xXJSONDeruloXx I think we'll need to include the built .so in here, we don't rebuild when making the APK |
|
We should build from source wherever possible instead of bundling bins imo. @utkarshdalal |
|
yes but we don't have the source for winlator11.so Please do include the built file in the PR and I can merge |
|
@utkarshdalal done, but aside from winlator11.so I want to revisit building from source as standard, the less opaque the codebase the better |
|
Sure, I agree it's a better practice. The source for most of the .sos is what is in the winlator bionic repo |
|
Yet if we make changes over time but then bundle the bins even if source is in the codebase it's unclear where a bin blobs diff is from unless you trudge through merges. Especially when pr gets squashed |
|
Yup, I'm in agreement with you. Was just saying where we need to pull in the native code from :D |
|
Other than that, .so's added so lmk if lgtm! |
4 participants
https://discord.com/channels/1378308569287622737/1412756778159964201/1486379950238994554
Summary by cubic
Make xconnector epoll/poll waits resilient to EINTR so connections survive device suspend without stalls or drops. Adds a native
xconnectorpatchbridge and routes Java calls through it for reliable waiting and FD handling.Bug Fixes
epoll_wait,poll, andacceptonEINTR; log failures and ignore transientEAGAIN/EWOULDBLOCK.POLLERR/POLLHUP/POLLNVALas closed; ensurewaitForSocketReadexits on shutdown or error.epoll_ctladd failure; keepdoEpollIndefinitelyreturning consistently.epoll_ctlDEL on closed/not-found fds.Dependencies
xconnectorpatchJNI library (arm64-v8a,armeabi-v7a) and load viaXConnectorEpollNative; Java now calls through this bridge.externalNativeBuilddisabled; APK includes the prebuilt libs fromapp/src/main/jniLibs.Written for commit 9627218. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores