refactor: 重构核心，轻量化处理 PHP同步阻塞#3
Conversation
📝 WalkthroughWalkthroughThe server now runs connection tasks on a multi-threaded Tokio runtime, moves thread-safe handler bundles into those tasks, serializes PHP callback execution, applies HTTP callback timeouts, and routes WebSocket lifecycle callbacks through the shared blocking helper. ChangesPHP callback concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TokioTask
participant HTTPHandler
participant WebSocket
participant run_php_blocking
participant PHPCallback
Client->>TokioTask: send HTTP request or WebSocket frame
TokioTask->>HTTPHandler: process HTTP request
HTTPHandler->>run_php_blocking: invoke PHP callback with timeout
run_php_blocking->>PHPCallback: execute serialized callback
Client->>WebSocket: establish connection or send frame
WebSocket->>run_php_blocking: invoke open, message, or close callback
run_php_blocking->>PHPCallback: execute serialized callback
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@rust/src/http.rs`:
- Around line 293-299: Correct the response body length in the 504 Gateway
Timeout response constructed in the HTTP handling flow, matching Content-Length
to the actual body bytes. Also propagate the timeout state into the keep-alive
decision so timeout responses honor the existing Connection: close header and
terminate the socket.
- Around line 277-283: Update the timeout flow around run_php_blocking in the
HTTP response handling so the PHP call is executed by a genuinely asynchronous
future rather than being eagerly evaluated inside std::future::ready. Ensure
request_timeout begins applying before the blocking handler runs, allowing hung
handlers to reach the existing timeout/504 path and not hold PHP_CALL_LOCK
indefinitely.
- Around line 21-28: Remove the unsafe Send/Sync implementations for
HandlerBundle in rust/src/http.rs:21-28, keep PHP callback execution
thread-affine, and replace Tokio cross-thread movement with channel-based
handoff. Update the affected HTTP flow in rust/src/http.rs:107-138, runtime
paths in rust/src/runtime.rs:51-82 and 296-302, and websocket flows in
rust/src/websocket.rs:23-46, 108-111, 148-166, and 202-205 so
HandlerBundle/ZendCallable remains on its owning thread; the mutex must only
serialize execution, not justify cross-thread transfer.
In `@rust/src/websocket.rs`:
- Around line 158-160: Update the Message::Binary handling in the websocket
message flow to avoid converting invalid UTF-8 data into an empty string via
unwrap_or_default. Use a loss-aware conversion, explicitly reject invalid
frames, or pass the raw bytes/base64 through to the PHP block while preserving
valid payload content.
🪄 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: 994a979a-5e17-4593-8752-b6b473ca01fe
📒 Files selected for processing (3)
rust/src/http.rsrust/src/runtime.rsrust/src/websocket.rs
| // SAFETY: | ||
| // `ZendCallable` 内部只是指向 PHP 函数表的指针，进程生命周期内 | ||
| // 地址不变。我们通过 `runtime::PHP_CALL_LOCK` 全局互斥锁保证同一 | ||
| // 时刻只有一个线程进入 PHP 解释器，因此跨线程传递 `HandlerBundle` | ||
| // 是安全的。没有这个 `Send` impl，`tokio::spawn` 无法用它（它 | ||
| // 要求 `Future: Send`）。 | ||
| unsafe impl Send for HandlerBundle {} | ||
| unsafe impl Sync for HandlerBundle {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'PHP_ZTS|ZTS|ZendCallable|unsafe impl (Send|Sync)|run_php_blocking|new_multi_thread' .
fd -a -t f 'Cargo.toml|Cargo.lock|build.rs|config.m4' .Repository: watsonhaw5566/lychee-worker
Length of output: 15043
🌐 Web query:
ext-php-rs ZendCallable Send Sync thread safety PHP NTS ZTS Tokio block_in_place mutex
💡 Result:
In the context of the ext-php-rs library, managing thread safety between Rust and PHP requires strict adherence to PHP's execution model and the specific configuration (NTS vs. ZTS) [1][2][3]. ZendCallable and Thread Safety The ZendCallable type in ext-php-rs is explicitly marked as not implementing Send and not implementing Sync [4]. This is because PHP's internal callable representation is bound to the Zend engine's state, which is not thread-safe to access from multiple threads simultaneously [4][1]. You must not share or move ZendCallable instances between threads. PHP Threading Model (NTS vs. ZTS) - NTS (Non-Thread-Safe): PHP operates in a single-threaded request mode. Accessing PHP state (such as Zend globals) is only safe from the thread currently executing the request [1]. - ZTS (Zend Thread Safety): PHP uses thread-local storage (TSRM) to maintain engine state per thread [1][5]. To safely interact with PHP from an external thread (e.g., one spawned by Rust), you must use the PhpThreadGuard RAII wrapper [5]. This ensures that the thread is properly registered with PHP's thread-local storage before executing any code that touches the PHP engine [5]. Tokio and Blocking When integrating Tokio with PHP code: - PHP Request Threads: If you are running inside a standard PHP request handler, you are in a synchronous, single-threaded context [1]. Using blocking calls (like std::sync::Mutex or blocking tokio operations) within a PHP function can block the entire PHP worker thread, preventing it from handling further requests or finishing its current execution. - block_in_place and spawn_blocking: If you are inside an asynchronous Tokio task (e.g., using #[php_async_impl]) and need to execute synchronous PHP code or block on a operation, use tokio::task::block_in_place or tokio::task::spawn_blocking [6][7][8]. This offloads the blocking work, preventing the async executor from stalling [8]. - Mutexes: Use caution with mutexes. If a thread holds a mutex while also needing to interact with the PHP engine (especially in ZTS mode), ensure that you do not hold that lock while performing operations that might re-enter PHP or perform long-running blocking tasks, as this is a common source of deadlocks [9]. If you must share data between Rust threads, perform the sharing at the Rust level using standard Rust concurrency primitives, and only enter the PHP context through appropriate thread-safe handles or guards when ready to execute PHP-specific logic [1][5].
Citations:
- 1: https://docs.rs/ext-php-rs/latest/ext_php_rs/zend/struct.ModuleGlobals.html
- 2: https://docs.rs/ext-php-rs/latest/ext_php_rs/
- 3: https://docs.rs/ext-php-rs/latest/ext_php_rs/constant.PHP_ZTS.html
- 4: https://docs.rs/ext-php-rs/latest/ext_php_rs/types/struct.ZendCallable.html
- 5: https://ext-php.rs/advanced/worker_mode.html
- 6: https://ext-php.rs/advanced/async_impl.html
- 7: https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html
- 8: https://docs.rs/tokio/latest/tokio/task/fn.block_in_place.html
- 9:
block_in_place+block_on+tokio::sync::Mutexmay lead to a deadlock tokio-rs/tokio#7892
Remove the Send/Sync impls unless PHP callbacks stay on one owning thread. ext-php-rs treats ZendCallable as non-Send/non-Sync, and the current Tokio setup can move these bundles across worker threads; the mutex only serializes access, it does not make Zend/PHP state safe to transfer. Keep the PHP-owned execution path thread-affine and hand off work via channels instead.
📍 Affects 3 files
rust/src/http.rs#L21-L28(this comment)rust/src/http.rs#L107-L138rust/src/runtime.rs#L51-L82rust/src/runtime.rs#L296-L302rust/src/websocket.rs#L23-L46rust/src/websocket.rs#L108-L111rust/src/websocket.rs#L148-L166rust/src/websocket.rs#L202-L205
🤖 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 `@rust/src/http.rs` around lines 21 - 28, Remove the unsafe Send/Sync
implementations for HandlerBundle in rust/src/http.rs:21-28, keep PHP callback
execution thread-affine, and replace Tokio cross-thread movement with
channel-based handoff. Update the affected HTTP flow in
rust/src/http.rs:107-138, runtime paths in rust/src/runtime.rs:51-82 and
296-302, and websocket flows in rust/src/websocket.rs:23-46, 108-111, 148-166,
and 202-205 so HandlerBundle/ZendCallable remains on its owning thread; the
mutex must only serialize execution, not justify cross-thread transfer.
| let response = match tokio::time::timeout( | ||
| request_timeout, | ||
| std::future::ready(crate::runtime::run_php_blocking(move || { | ||
| crate::php_api::call_on_http(http_handler, &method_c, &path_c, &raw_headers, &body) | ||
| })), | ||
| ) | ||
| .await |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C5 'time::timeout|future::ready|run_php_blocking|block_in_place' rust/srcRepository: watsonhaw5566/lychee-worker
Length of output: 10549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- http.rs relevant block ---'
sed -n '268,286p' rust/src/http.rs
echo
echo '--- runtime.rs run_php_blocking ---'
sed -n '58,82p' rust/src/runtime.rs
echo
echo '--- any other timeout+run_php_blocking patterns ---'
rg -n -C3 'timeout\(|run_php_blocking\(' rust/src | sed -n '1,220p'
echo
echo '--- evaluation-order probe (Python) ---'
python3 - <<'PY'
events = []
def run_php_blocking():
events.append("run_php_blocking")
return "php-result"
def ready(x):
events.append(f"ready({x})")
return ("future", x)
def timeout(d, fut):
events.append(f"timeout({d}, {fut})")
return "timeout-await"
# Mimic timeout(request_timeout, ready(run_php_blocking()))
result = timeout(5, ready(run_php_blocking()))
events.append(f"result={result}")
print("\n".join(events))
PYRepository: watsonhaw5566/lychee-worker
Length of output: 6262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '283,306p' rust/src/http.rsRepository: watsonhaw5566/lychee-worker
Length of output: 914
Move the PHP call out of future::ready
run_php_blocking(...) is evaluated before tokio::time::timeout starts polling, so the deadline only wraps an already-completed ready future. A hung handler can still hold PHP_CALL_LOCK indefinitely, and this branch never reaches the 504 path.
🤖 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 `@rust/src/http.rs` around lines 277 - 283, Update the timeout flow around
run_php_blocking in the HTTP response handling so the PHP call is executed by a
genuinely asynchronous future rather than being eagerly evaluated inside
std::future::ready. Ensure request_timeout begins applying before the blocking
handler runs, allowing hung handlers to reach the existing timeout/504 path and
not hold PHP_CALL_LOCK indefinitely.
| "HTTP/1.1 504 Gateway Timeout\r\n\ | ||
| Content-Type: text/plain; charset=utf-8\r\n\ | ||
| Content-Length: 19\r\n\ | ||
| Connection: close\r\n\ | ||
| \r\n\ | ||
| Gateway Timeout\n" | ||
| .to_string() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the malformed 504 response and force connection closure.
Gateway Timeout\n is 16 bytes, not 19. Additionally, propagate a timeout flag to the keep-alive decision so this Connection: close response actually closes the socket.
Proposed content-length fix
- Content-Length: 19\r\n\
+ Content-Length: 16\r\n\🤖 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 `@rust/src/http.rs` around lines 293 - 299, Correct the response body length in
the 504 Gateway Timeout response constructed in the HTTP handling flow, matching
Content-Length to the actual body bytes. Also propagate the timeout state into
the keep-alive decision so timeout responses honor the existing Connection:
close header and terminate the socket.
| Message::Binary(data) => { | ||
| let payload = String::from_utf8(data).unwrap_or_default(); | ||
| crate::php_api::call_on_ws_message( | ||
| ws_message_handler, | ||
| &conn_id, | ||
| &payload, | ||
| ); | ||
| crate::runtime::run_php_blocking(|| { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not silently replace invalid binary payloads with an empty message.
Use lossy conversion, explicitly reject non-UTF-8 frames, or expose bytes/base64 to PHP.
Minimal loss-aware fix
- let payload = String::from_utf8(data).unwrap_or_default();
+ let payload = String::from_utf8_lossy(&data).into_owned();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Message::Binary(data) => { | |
| let payload = String::from_utf8(data).unwrap_or_default(); | |
| crate::php_api::call_on_ws_message( | |
| ws_message_handler, | |
| &conn_id, | |
| &payload, | |
| ); | |
| crate::runtime::run_php_blocking(|| { | |
| Message::Binary(data) => { | |
| let payload = String::from_utf8_lossy(&data).into_owned(); | |
| crate::runtime::run_php_blocking(|| { |
🤖 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 `@rust/src/websocket.rs` around lines 158 - 160, Update the Message::Binary
handling in the websocket message flow to avoid converting invalid UTF-8 data
into an empty string via unwrap_or_default. Use a loss-aware conversion,
explicitly reject invalid frames, or pass the raw bytes/base64 through to the
PHP block while preserving valid payload content.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes