feat(http): add QuickJS script handler - #871
Merged
Merged
Conversation
ithewei
force-pushed
the
js-http-script-handler
branch
from
August 21, 2026 06:42
a767821 to
65bb368
Compare
Expose stable JS-layer run helpers mirroring hvlua_dofile/hvlua_dostring so callers can run a script on a loop's per-loop QuickJS runtime without touching the task/runtime plumbing. The script body is wrapped in an async function, so top-level await works; global require/print/arg are installed. Returns 1 when finished synchronously, 0 when pending on async work (caller runs the loop), <0 on setup/load/runtime error, with an optional exit_code set on reject/timeout. Thin examples/hvjs.cpp down to a hvlua.cpp-style runner that just creates the loop, publishes TLS, and calls hvjs_dofile.
- Reuse a per-loop AsyncHttpClient owned by the runtime (mirrors the lua binding) instead of building/tearing one per request; drop the per-request defer-release machinery. - Add a generic HvJsCleanup hook list on HvJsRuntime (hvjs_runtime_add_cleanup) so bindings can own loop-bound state for the loop lifetime. - Store the raw hloop_t* on the runtime; tasks no longer cache loop/loop_ptr, recovering the EventLoopPtr on demand via currentThreadEventLoopPtr on the loop thread (same pattern as lua). Collapse the C++/raw scheduling branches to a single raw hloop path. - Rename hvjs_task_set_runtime to hvjs_runtime_add_task and fold the timeout arming into it (timeout_ms arg); route HttpJsHandler teardown through hvjs_task_close. - Shrink the exported surface: keep drain/watch/cancel/start_timeout internal to hvjs.cpp; hvjs.h now exposes a coherent runtime/task/promise API.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Per-request timeout attribution, native resource bounds, cancellation, and public loop lifecycle behavior have unresolved correctness and reliability issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
js/hvjs_http.cpp:432
- Every incoming WebSocket message is retained without a queue limit when JavaScript is not currently awaiting
recv(). A peer can flood this C++ queue, which is outside QuickJS's 64 MB limit, and exhaust process memory before the request timeout. Add a bounded inbox/backpressure policy and close or reject on overflow.
- Files reviewed: 44/44 changed files
- Comments generated: 9
- Review effort level: Balanced
Comment on lines
+376
to
+379
| hloop_t* loop = hvjs_task_loop(task); | ||
| if (loop && hloop_status(loop) == HLOOP_STATUS_RUNNING) { | ||
| hloop_stop(loop); | ||
| } |
Comment on lines
+683
to
+687
| HvJsTaskScope scope(task); | ||
| JSContext* job_ctx = NULL; | ||
| JSRuntime* rt = task->runtime->rt; | ||
| while (JS_IsJobPending(rt)) { | ||
| int rc = JS_ExecutePendingJob(rt, &job_ctx); |
Comment on lines
+53
to
+56
| void cancel(const char* reason) override { | ||
| if (req) { | ||
| req->Cancel(); | ||
| } |
Comment on lines
+230
to
+232
| msg.qos = client->message.qos; | ||
| state->inbox.push_back(std::move(msg)); | ||
| js_mqtt_try_deliver(state); |
Comment on lines
+355
to
358
| std::shared_ptr<PendingRequest> request = pending.front(); | ||
| pending.pop_front(); | ||
| cancelTimeout(request); | ||
| invokeRequestCallback(request, code); |
- Free the property-enum atoms (JS_FreePropertyEnum) when copying request headers; the old js_free(tab) leaked one atom per header per request into the shared runtime. - Cap array-form redis commands at a bounded argument count so a sparse array with a huge length cannot spin the native build loop (uninterruptible inside the C callback) and grow RedisCommand unboundedly. - On script-load failure, log the raw error server-side and return the generic 'javascript handler error' body instead of leaking the filesystem error. - Strengthen the mqtt test to assert a non-empty err, proving connect() actually rejected.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Request lifecycle, timeout attribution, standalone loop startup, and unbounded native queues have unresolved correctness and reliability issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (2) — in code that hasn't changed since the last review.
scripts/unittest.sh:83
- Failures from
redis_async_client_testand the other native Redis tests are still ignored: the loop continues and a later successful command can makeunittest.shexit zero. This is especially problematic for the pending-queue reentrancy change in this PR, because CI can miss its regression test failing. Propagate every Redis test's exit status instead of special-casing onlylua_redis_test.
js/hvjs.cpp:430 - This API evaluates the script before the caller starts the loop, yet
hv/wsandhv/rediscallEventLoopThread::start()and require an already-running external loop. For example,ws.connect()starts the supplied loop on its own worker; the provided CLI then sees thatEventLoopas already running, returns fromloop->run()immediately, and exits while the script is still pending. Run initial evaluation from a callback after the caller-owned loop is running (and stop it on setup failure), or change these bindings so they never start that loop themselves.
js/hvjs.cpp:687
- The runtime-wide job queue can execute a promise continuation belonging to a different
JSContextthantask, butHvJsTaskScopeleavesruntime->current_taskset to the task that happened to trigger this drain. The interrupt handler therefore applies the wrong request's deadline: an older request can receive extra CPU time, a newer one can be interrupted early, and a job drained while a timeout-disabled task is current can spin forever. Track the executing job context/deadline at runtime level (or isolate job queues/runtimes) so CPU interruption does not depend on the drain initiator.
http/server/HttpJsHandler.cpp:213 - Reject status values outside the valid HTTP range. Returning
0(orNaN, which converts to zero) currently setsresponse->status_codetoHTTP_STATUS_NEXT; the synchronous handler then returns zero after its task has already been destroyed, soHttpHandlerwaits for an asynchronous response that will never be sent.
if (JS_IsNumber(value)) {
int32_t status = 0;
if (JS_ToInt32(js, &status, value) == 0 && ctx->response->status_code == HTTP_STATUS_OK) {
ctx->response->status_code = (http_status)status;
}
js/hvjs_mqtt.cpp:231
- The MQTT inbox is unbounded when
PUBLISHtraffic outpacesrecv(). Payloads are stored in nativestd::strings outside QuickJS's memory accounting, so the documented runtime memory limit offers no protection and a broker can exhaust server memory during a long-lived request. Enforce a message/byte limit and disconnect or reject on overflow.
js/hvjs_redis.cpp:121 - A cap of 1,048,576 still allows a sparse array to force a million native property lookups and string allocations on the IO thread, where the QuickJS interrupt handler cannot run. This can block all requests on that loop well beyond
timeout_mswhile also allocating substantial native memory. Use a small protocol-appropriate argument limit or check the task deadline incrementally.
redis/AsyncRedisClient.cpp:358 - Add a regression test that re-enters the client from a completion/failure callback (for example, calling
command()orstop()while another request is pending). The existing Redis test suite covers disconnect-after-reply, but its callback only records results, so it would not catch restoring the original pop-after-callback ordering that this change specifically fixes.
- Files reviewed: 44/44 changed files
- Comments generated: 2
- Review effort level: Balanced
Comment on lines
+99
to
+102
| int32_t status = 0; | ||
| if (JS_ToInt32(js, &status, argv[0]) != 0) return JS_EXCEPTION; | ||
| task->ctx->response->status_code = (http_status)status; | ||
| return JS_NewInt32(js, status); |
Comment on lines
+430
to
+432
| state->client->onmessage = [state](const std::string& msg) { | ||
| state->inbox.push_back(msg); | ||
| js_ws_try_deliver(state); |
ctx.status(0) (or a handler returning 0/NaN) stored 0 into response->status_code, which aliases HTTP_STATUS_NEXT. A completed synchronous handler then reported a deferred async response with no task left to send it, hanging the request. Reject out-of-range status in ctx.status() (RangeError) and ignore it in the return-value mapping, so the status stays 200. Add a regression covering ctx.status(0).
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
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.
Summary
WITH_JS/ QuickJS support and installHttpJsHandler.hplus publichv/hvjs.hwhen enabled.HttpJsHandlerplusHttpScriptHandlerdispatch for.jsscripts.js/with controlled modules:hv,hv/http,hv/ws,hv/redis, andhv/mqtt; no Node.js/npm module loading.hloop_tviahloop_set_js_runtime/hloop_js_runtime, with oneJSContextper HTTP request.async/await, including request-level timeout, built-in QuickJS memory/stack limits, and interrupt handling for CPU-bound scripts.ws/mqtt recv()remain message-driven and rely on requesttimeout_msas the HTTP lifecycle safety budget.AsyncRedisClientpending request callback reentrancy by popping completed/failed requests before invoking callbacks.--with-jsshared/static libhv build instead of the previous static-only JS pass plus second libhv rebuild.Testing
git diff --checkbellard/quickjsat04be246001599f5995fa2f2d8c91a0f198d3f34c, appendCFLAGS_OPT+=-fPIC, buildlibquickjs.a; verified compile commands keepCONFIG_VERSIONand include-fPIC..github/workflows/CI.ymlshell block syntax: extracted eachrun: |block and ranbash -n..github/workflows/CI.ymlYAML parse:ruby -e 'require "yaml"; YAML.load_file(".github/workflows/CI.yml"); puts "yaml ok"'.cmake -S . -B /tmp/libhv-js-final-check -DWITH_JS=ON -DWITH_HTTP=ON -DWITH_HTTP_SERVER=ON -DWITH_HTTP_CLIENT=ON -DWITH_REDIS=ON -DWITH_MQTT=ON -DBUILD_SHARED=OFF -DBUILD_STATIC=ON -DBUILD_EXAMPLES=ON -DBUILD_UNITTEST=ON -DQUICKJS_ROOT=/opt/homebrew/opt/quickjscmake --build /tmp/libhv-js-final-check --target hvjs http_js_handler_test http_js_redis_test http_js_ws_test http_js_mqtt_test -j2/tmp/libhv-js-final-check/bin/http_js_handler_test && /tmp/libhv-js-final-check/bin/http_js_redis_test && /tmp/libhv-js-final-check/bin/http_js_ws_test && /tmp/libhv-js-final-check/bin/http_js_mqtt_test && /tmp/libhv-js-final-check/bin/hvjs examples/js/sleep.jscmake --install /tmp/libhv-js-final-check --prefix /tmp/libhv-install-js-finalfind_package(libhv REQUIRED CONFIG),#include <hv/hvjs.h>, linklibhv::hv_static, build and run with-DCMAKE_PREFIX_PATH=/tmp/libhv-install-js-final -DQUICKJS_ROOT=/opt/homebrew/opt/quickjsmake libhv hvjs unittest WITH_JS=yes WITH_HTTP=yes WITH_MQTT=yes WITH_REDIS=yes -j2DYLD_LIBRARY_PATH=$(pwd)/lib:$DYLD_LIBRARY_PATH bash scripts/unittest.shmake http_server_test WITH_JS=yes WITH_HTTP=yes -j2