From 217c8fe8d064847f25081191efe05fbd492b4a0a Mon Sep 17 00:00:00 2001 From: Aleksandr Borovskii Date: Tue, 22 Sep 2026 22:34:53 +0200 Subject: [PATCH 1/4] fix: emit end promptly for serialized requests on Node.js 26 Multi::OnSocket and Multi::OnTimeout enter JS from a libuv callback, that is, from outside any JS context. Multi::CallOnMessageCallback settles the promise perform() returns from there, and Node.js only performs a microtask checkpoint when a callback scope is closed. Only a HandleScope was being opened, so the continuation that emits `end` was not scheduled until something else drained the microtask queue - and for a caller awaiting one request at a time, that is the next tick of libcurl's own multi timer. On Node.js 26 / Linux this made every serialized request take ~1000ms even though the transfer itself completed in under 3ms: headers and body arrived at ~2ms and CURLINFO_TOTAL_TIME_T reported 0.5-2.8ms. Issuing requests concurrently masked it, because the other in-flight transfers kept waking the loop. Node.js 22, 24 and 25 were unaffected, as was macOS. Open a Napi::CallbackScope in both libuv callbacks, backed by a single long-lived AsyncContext per Multi so that no napi_async_init happens per socket event. Easy::CallSocketEvent, the only other libuv -> JS entry point, already used MakeCallback with an async context and needed no change. Serialized throughput on node:26-alpine goes from 1.3 to 3046 req/s (p50 995.7ms -> 0.29ms). Node.js 24 is unchanged within noise. --- src/Multi.cc | 26 ++++++++++ src/Multi.h | 5 ++ test/curl/serializedRequests.spec.ts | 76 ++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+) create mode 100644 test/curl/serializedRequests.spec.ts diff --git a/src/Multi.cc b/src/Multi.cc index b4027d483..4c64e596f 100644 --- a/src/Multi.cc +++ b/src/Multi.cc @@ -78,6 +78,10 @@ Multi::Multi(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info), id // We need to keep the reference alive for the duration of the timer. this->Ref(); + // Used by OnSocket / OnTimeout to open a callback scope when entering JS from + // libuv. Created once here rather than per event. + this->asyncContext = std::make_unique(env, "node-libcurl:Multi"); + // Enable notification API if requested and supported if (shouldUseNotificationsApi) { #if NODE_LIBCURL_VER_GE(8, 17, 0) @@ -168,6 +172,9 @@ void Multi::Dispose() { this->mh = nullptr; } + // Must happen while the environment is still valid: napi_async_destroy. + this->asyncContext.reset(); + curl->AdjustHandleMemory(CURL_HANDLE_TYPE_MULTI, -1); } @@ -840,6 +847,17 @@ void Multi::NotifyCallback(CURLM* multi, unsigned int notification, CURL* easy, UV_TIMER_CB(Multi::OnTimeout) { Multi* obj = static_cast(timer->data); + // This runs from libuv, that is, from outside any JS context. Node.js only + // performs a microtask checkpoint when a callback scope is closed, so without + // one the promise CallOnMessageCallback settles below stays queued until + // something else happens to drain the microtask queue - which, for a caller + // awaiting one request at a time, is the next tick of this very timer. + if (!obj->asyncContext) return; + + Napi::Env env = obj->Env(); + Napi::HandleScope handleScope(env); + Napi::CallbackScope callbackScope(env, *obj->asyncContext); + NODE_LIBCURL_DEBUG_LOG(obj, "Multi::OnTimeout", ""); // Check comment on node_libcurl.cc @@ -867,6 +885,14 @@ void Multi::OnSocket(uv_poll_t* handle, int status, int events) { Multi::CurlSocketContext* ctx = static_cast(handle->data); + // See the comment in Multi::OnTimeout - a callback scope is required here for + // the microtask checkpoint to run. + if (!ctx->multi->asyncContext) return; + + Napi::Env env = ctx->multi->Env(); + Napi::HandleScope handleScope(env); + Napi::CallbackScope callbackScope(env, *ctx->multi->asyncContext); + NODE_LIBCURL_DEBUG_LOG(ctx->multi, "Multi::OnSocket", "events: " + std::to_string(events)); // Check comment on node_libcurl.cc diff --git a/src/Multi.h b/src/Multi.h index a8e5250fd..5d0139df3 100644 --- a/src/Multi.h +++ b/src/Multi.h @@ -85,6 +85,11 @@ class Multi : public Napi::ObjectWrap { napi_async_cleanup_hook_handle removeHandle; uint64_t id; + // Both libuv callbacks below (OnSocket / OnTimeout) call into JS from outside + // any JS context, so they need a callback scope. One long-lived async context + // per Multi is enough, and avoids a napi_async_init per socket event. + std::unique_ptr asyncContext; + std::map socketContextMap; // Notification API support (libcurl >= 8.17.0) diff --git a/test/curl/serializedRequests.spec.ts b/test/curl/serializedRequests.spec.ts new file mode 100644 index 000000000..dfe848744 --- /dev/null +++ b/test/curl/serializedRequests.spec.ts @@ -0,0 +1,76 @@ +/** + * Copyright (c) Jonathan Cardoso Machado. All Rights Reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +import { describe, beforeAll, afterAll, it, expect } from 'vitest' + +import { createServer } from '../helper/server' +import { Curl } from '../../lib' +import { withCommonTestOptions } from '../helper/commonOptions' + +let serverInstance: ReturnType + +// Regression guard for requests awaited one at a time. +// +// Multi::OnSocket and Multi::OnTimeout enter JS from a libuv callback. The +// promise `perform()` settles is resolved from there, and Node.js only runs a +// microtask checkpoint when a callback scope is closed. Without one, the +// continuation that emits `end` was not scheduled until the next tick of +// libcurl's multi timer, making every serialized request take ~1s on Node.js 26 +// (Linux). Concurrency hid it, because other transfers kept the loop busy. +describe('Curl', () => { + beforeAll(async () => { + serverInstance = createServer() + serverInstance.app.get('/', (_req, res) => { + res.send('Hello World!') + }) + await serverInstance.listen() + }) + + afterAll(async () => { + await serverInstance.close() + serverInstance.app._router.stack.pop() + }) + + describe('perform', () => { + const performOnce = () => + new Promise((resolve, reject) => { + const startedAt = process.hrtime.bigint() + const curl = new Curl() + withCommonTestOptions(curl) + curl.setOpt('URL', serverInstance.url) + + curl.on('end', () => { + const elapsedMs = Number(process.hrtime.bigint() - startedAt) / 1e6 + curl.close() + resolve(elapsedMs) + }) + curl.on('error', (error) => { + curl.close() + reject(error) + }) + + curl.perform() + }) + + it('should emit end without waiting for the multi timer when requests are awaited one at a time', async () => { + // warm-up: the very first request pays for connection setup and JIT + await performOnce() + + const latencies: number[] = [] + for (let i = 0; i < 10; i += 1) { + latencies.push(await performOnce()) + } + + // Against a local server each request is sub-millisecond. The bug pinned + // every one of them at libcurl's multi timer period (~1000ms), so a + // generous threshold still separates the two regimes decisively. + const sorted = [...latencies].sort((a, b) => a - b) + const median = sorted[Math.floor(sorted.length / 2)] + + expect(median).toBeLessThan(250) + }) + }) +}) From 4d4739a945c95110a34ffb9bde96a31d159d5cee Mon Sep 17 00:00:00 2001 From: Aleksandr Borovskii Date: Wed, 23 Sep 2026 01:29:42 +0200 Subject: [PATCH 2/4] test: cover the polling drain path in the serialized request guard The default Multi drains completions from Multi::NotifyCallback when built against libcurl >= 8.17, and directly from OnSocket / OnTimeout otherwise. Both sit inside the callback scopes the previous commit added, but only the notification-enabled path was exercised here, leaving the polling fallback covered incidentally by whichever CI leg happens to build against an older libcurl. Parameterize the helper with an optional Multi and add a case pinned to `new Multi({ shouldUseNotificationsApi: false })`, closing it afterwards. Both cases fail on the unfixed addon (8.4s and 7.0s for 11 requests) and pass with it. --- test/curl/serializedRequests.spec.ts | 41 +++++++++++++++++++++------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/test/curl/serializedRequests.spec.ts b/test/curl/serializedRequests.spec.ts index dfe848744..d8e8ada8c 100644 --- a/test/curl/serializedRequests.spec.ts +++ b/test/curl/serializedRequests.spec.ts @@ -7,7 +7,7 @@ import { describe, beforeAll, afterAll, it, expect } from 'vitest' import { createServer } from '../helper/server' -import { Curl } from '../../lib' +import { Curl, Multi } from '../../lib' import { withCommonTestOptions } from '../helper/commonOptions' let serverInstance: ReturnType @@ -35,13 +35,17 @@ describe('Curl', () => { }) describe('perform', () => { - const performOnce = () => + const performOnce = (multi?: Multi) => new Promise((resolve, reject) => { const startedAt = process.hrtime.bigint() const curl = new Curl() withCommonTestOptions(curl) curl.setOpt('URL', serverInstance.url) + if (multi) { + curl.setMulti(multi) + } + curl.on('end', () => { const elapsedMs = Number(process.hrtime.bigint() - startedAt) / 1e6 curl.close() @@ -55,22 +59,39 @@ describe('Curl', () => { curl.perform() }) - it('should emit end without waiting for the multi timer when requests are awaited one at a time', async () => { + // Against a local server each request is sub-millisecond. The bug pinned + // every one of them at libcurl's multi timer period (~1000ms), so a + // generous threshold still separates the two regimes decisively. + const medianSerializedLatency = async (multi?: Multi) => { // warm-up: the very first request pays for connection setup and JIT - await performOnce() + await performOnce(multi) const latencies: number[] = [] for (let i = 0; i < 10; i += 1) { - latencies.push(await performOnce()) + latencies.push(await performOnce(multi)) } - // Against a local server each request is sub-millisecond. The bug pinned - // every one of them at libcurl's multi timer period (~1000ms), so a - // generous threshold still separates the two regimes decisively. const sorted = [...latencies].sort((a, b) => a - b) - const median = sorted[Math.floor(sorted.length / 2)] + return sorted[Math.floor(sorted.length / 2)] + } - expect(median).toBeLessThan(250) + it('should emit end without waiting for the multi timer when requests are awaited one at a time', async () => { + expect(await medianSerializedLatency()).toBeLessThan(250) + }) + + // The default Multi drains completions from Multi::NotifyCallback when built + // against libcurl >= 8.17, and straight from OnSocket / OnTimeout otherwise. + // Both are inside the callback scopes this guards, so pin the polling + // fallback explicitly instead of relying on a libcurl < 8.17 build to be the + // only thing exercising it. + it('should emit end without waiting for the multi timer when the notifications API is disabled', async () => { + const multi = new Multi({ shouldUseNotificationsApi: false }) + + try { + expect(await medianSerializedLatency(multi)).toBeLessThan(250) + } finally { + multi.close() + } }) }) }) From 31061503180a8e9d4868459a950e8d9da5111582 Mon Sep 17 00:00:00 2001 From: Aleksandr Borovskii Date: Sun, 25 Oct 2026 01:35:23 +0200 Subject: [PATCH 3/4] fix: keep the multi async context alive while its callback scope is open Calling multi.close() from inside onMessage, as examples/04-multi.js does, runs Dispose() while the callback scope opened by OnSocket / OnTimeout is still open. Dispose() reset the only reference to the async context, so napi_async_destroy freed it under the open scope. Since Node.js 25, napi_open_callback_scope pushes a pointer to the context's resource onto the async resource stack, and anything that read it before the scope closed (executionAsyncResource(), AsyncLocalStorage) read freed memory: it returned undefined, or crashed under Guard Malloc. Hold the context in a shared_ptr and take a local copy for the lifetime of each scope, so it is destroyed only after the scope closes. The regression test runs on the polling path. With the notifications API, onMessage runs inside libcurl's notify callback, and curl_multi_cleanup fails there with CURLM_RECURSIVE_API_CALL. --- src/Multi.cc | 18 +++++++---- src/Multi.h | 3 +- test/curl/multi.spec.ts | 67 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 7 deletions(-) create mode 100644 test/curl/multi.spec.ts diff --git a/src/Multi.cc b/src/Multi.cc index 4c64e596f..c9e13ee6a 100644 --- a/src/Multi.cc +++ b/src/Multi.cc @@ -80,7 +80,7 @@ Multi::Multi(const Napi::CallbackInfo& info) : Napi::ObjectWrap(info), id // Used by OnSocket / OnTimeout to open a callback scope when entering JS from // libuv. Created once here rather than per event. - this->asyncContext = std::make_unique(env, "node-libcurl:Multi"); + this->asyncContext = std::make_shared(env, "node-libcurl:Multi"); // Enable notification API if requested and supported if (shouldUseNotificationsApi) { @@ -852,11 +852,16 @@ UV_TIMER_CB(Multi::OnTimeout) { // one the promise CallOnMessageCallback settles below stays queued until // something else happens to drain the microtask queue - which, for a caller // awaiting one request at a time, is the next tick of this very timer. - if (!obj->asyncContext) return; + // + // The local copy must outlive callbackScope: close() from inside onMessage + // resets obj->asyncContext while the scope is still open, and since Node.js 25 + // the async resource stack points into the context. + std::shared_ptr asyncContext = obj->asyncContext; + if (!asyncContext) return; Napi::Env env = obj->Env(); Napi::HandleScope handleScope(env); - Napi::CallbackScope callbackScope(env, *obj->asyncContext); + Napi::CallbackScope callbackScope(env, *asyncContext); NODE_LIBCURL_DEBUG_LOG(obj, "Multi::OnTimeout", ""); @@ -886,12 +891,13 @@ void Multi::OnSocket(uv_poll_t* handle, int status, int events) { Multi::CurlSocketContext* ctx = static_cast(handle->data); // See the comment in Multi::OnTimeout - a callback scope is required here for - // the microtask checkpoint to run. - if (!ctx->multi->asyncContext) return; + // the microtask checkpoint to run, and the local copy keeps its context alive. + std::shared_ptr asyncContext = ctx->multi->asyncContext; + if (!asyncContext) return; Napi::Env env = ctx->multi->Env(); Napi::HandleScope handleScope(env); - Napi::CallbackScope callbackScope(env, *ctx->multi->asyncContext); + Napi::CallbackScope callbackScope(env, *asyncContext); NODE_LIBCURL_DEBUG_LOG(ctx->multi, "Multi::OnSocket", "events: " + std::to_string(events)); diff --git a/src/Multi.h b/src/Multi.h index 5d0139df3..ad37b7a18 100644 --- a/src/Multi.h +++ b/src/Multi.h @@ -88,7 +88,8 @@ class Multi : public Napi::ObjectWrap { // Both libuv callbacks below (OnSocket / OnTimeout) call into JS from outside // any JS context, so they need a callback scope. One long-lived async context // per Multi is enough, and avoids a napi_async_init per socket event. - std::unique_ptr asyncContext; + // Shared so an open scope can keep it alive across close() (see OnTimeout). + std::shared_ptr asyncContext; std::map socketContextMap; diff --git a/test/curl/multi.spec.ts b/test/curl/multi.spec.ts new file mode 100644 index 000000000..115fc4ced --- /dev/null +++ b/test/curl/multi.spec.ts @@ -0,0 +1,67 @@ +/** + * Copyright (c) Jonathan Cardoso Machado. All Rights Reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +import { executionAsyncResource } from 'async_hooks' +import { describe, beforeAll, afterAll, it, expect } from 'vitest' + +import { createServer } from '../helper/server' +import { Easy, Multi } from '../../lib' +import { withCommonTestOptions } from '../helper/commonOptions' + +let serverInstance: ReturnType + +describe('Multi', () => { + beforeAll(async () => { + serverInstance = createServer() + serverInstance.app.get('/', (_req, res) => { + res.send('Hello World!') + }) + await serverInstance.listen() + }) + + afterAll(async () => { + await serverInstance.close() + serverInstance.app._router.stack.pop() + }) + + describe('close', () => { + // onMessage runs inside the callback scope opened by OnSocket / OnTimeout, + // and since Node.js 25 the async resource stack points into that scope's + // async context, so the context must outlive close(). Reading the stack + // used to return garbage or crash the process. + // + // Pinned to the polling path: with the notifications API, onMessage runs + // inside libcurl's notify callback, where curl_multi_cleanup fails with + // CURLM_RECURSIVE_API_CALL. + it('should keep the async resource valid when called from inside onMessage', async () => { + const resourceAfterClose = await new Promise( + (resolve, reject) => { + const multi = new Multi({ shouldUseNotificationsApi: false }) + const easy = new Easy() + withCommonTestOptions(easy) + easy.setOpt('URL', serverInstance.url) + easy.setOpt('WRITEFUNCTION', (_buf, size, nmemb) => size * nmemb) + + multi.onMessage((error, handle) => { + multi.removeHandle(handle) + handle.close() + multi.close() + + if (error) { + reject(error) + return + } + + resolve(executionAsyncResource()) + }) + multi.addHandle(easy) + }, + ) + + expect(typeof resourceAfterClose).toBe('object') + }) + }) +}) From 7579a55e0b7d25255310118704196b09bf0ea70a Mon Sep 17 00:00:00 2001 From: Aleksandr Borovskii Date: Sun, 25 Oct 2026 01:35:27 +0200 Subject: [PATCH 4/4] docs: add changelog entry for the node.js 26 completion dispatch fix --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index edcf65ca4..1e2baf5a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Breaking Change ### Fixed +- Fixed `end` (and the promise returned by `Multi#perform`) not being delivered on Node.js 26 until some other JavaScript callback ran, which made requests awaited one at a time take ~1s each (libcurl's multi timer period) or never complete in an otherwise idle process. `Multi::OnSocket` and `Multi::OnTimeout` called into JavaScript from libuv without a callback scope, so the microtask queue was not drained afterwards. Before Node.js 26.4.0 an unrelated drain in `CheckImmediate` hid this ([nodejs/node#62969](https://github.com/nodejs/node/pull/62969)). Both now open a `Napi::CallbackScope`. As a side effect, `process.nextTick` callbacks and microtasks queued from inside data, header, progress, and other easy callbacks now run once the libuv callback that dispatched them returns, instead of after each individual callback. This matches how Node.js core runs callbacks. ([#455](https://github.com/JCMais/node-libcurl/pull/455), fixes [#454](https://github.com/JCMais/node-libcurl/issues/454)) ### Added