JCMais / JCMais/node-libcurl

Possible `Multi` wrapper retention after `close()`

Open
#453 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
718
Forks
135
PR merge metrics
No merged PRs in 30d

Description

# Possible `Multi` wrapper retention after `close()`

I found a possible lifecycle leak where `Multi::close()` releases the curl multi handle but leaves the wrapper and timer alive until environment shutdown.

File: `src/Multi.cc`

Functions: `Multi::Multi`, `Multi::Close`, `Multi::Dispose`, `Multi::CloseTimerAsync`

Relevant code:

```cpp
uv_timer_init(loop, &this->timeout);
this->timeout.data = this;
// We need to keep the reference alive for the duration of the timer.
this->Ref();

napi_add_async_cleanup_hook(env, Multi::CleanupHookAsync, this, &removeHandle);
```

The reference is released only from the `uv_close` callback:

```cpp
uv_close(timeoutHandle, [](uv_handle_t* handle) {
uv_timer_t* timer = reinterpret_cast(handle);
Multi* multi = static_cast(timer->data);
napi_remove_async_cleanup_hook(multi->removeHandle);
multi->Unref();
});
```

But public `close()` only calls `Dispose()`:

```cpp
Napi::Value Multi::Close(const Napi::CallbackInfo& info) {
// ...
this->Dispose();
return env.Undefined();
}
```

`Dispose()` stops the timer and cleans up the curl multi handle, but does not
close the uv handle or release the strong wrapper reference:

```cpp
this->isOpen = false;
uv_timer_stop(&this->timeout);

if (this->mh) {
CURLMcode code = curl_multi_cleanup(this->mh);
assert(code == CURLM_OK);
this->mh = nullptr;
}
```

`CloseTimerAsync()` is called by the environment cleanup hook, so an explicitly
closed `Multi` can remain reachable through its own `Ref()` until environment
teardown.

Suggested fix: make `Multi::Close()` also initiate the timer close path, for
example by calling `CloseTimerAsync()` after `Dispose()`, while preserving the
existing duplicate-close guards.

Contributor guide

Open the contributing guide

Research direction

Start in src/Multi.cc by tracing Multi::Close, Multi::Dispose, Multi::CloseTimerAsync, and the constructor's timer setup. Confirm how explicit close interacts with the existing cleanup hook and duplicate-close guards; done means the wrapper is no longer retained after close while environment cleanup remains safe.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, node.js
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.