Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP Client's ConnectionPool erroneously leaks memory, then panics when releasing nodes at full capacity #16282

Closed
ghost opened this issue Jul 1, 2023 · 0 comments
Labels
bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@ghost
Copy link

ghost commented Jul 1, 2023

Zig Version

0.11.0-dev.3892+0a6cd257b

Steps to Reproduce and Observed Behavior

When the HTTP client's ConnectionPool attempts to release a used connection into its pool of free connections, if the pool of free connections is already at capacity, the ConnectionPool will free one of the open connections it already contains, but then it will not add the previously used connection to its pool to compensate for this removal. This results in an orphaned node which has its memory leaked.

In addition, each time this occurs the ConnectionPools "free_len" value will not be decremented even though the actual length of the free pool has decreased. Eventually, this leads to a situation where the ConnectionPool's free_len value will tell it that it already has the maximum desired amount of free connections when in fact it has zero free connections. At this point it will attempt to remove a node from its now empty free pool at which point the program will panic.

    // lib/std/http/Client.zig line #87
    pub fn release(pool: *ConnectionPool, client: *Client, node: *Node) void {
        pool.mutex.lock();
        defer pool.mutex.unlock();

        pool.used.remove(node);

        if (node.data.closing) {
            node.data.deinit(client);

            return client.allocator.destroy(node);
        }

        if (pool.free_len + 1 >= pool.free_size) {
            const popped = pool.free.popFirst() orelse unreachable; // <-- This is the line that causes the panic

            popped.data.deinit(client);

            return client.allocator.destroy(popped); // <-- This is the early return that I suspect is the sole culprit of the bug.
        }

        if (node.data.proxied) {
            pool.free.prepend(node);
        } else {
            pool.free.append(node);
        }

        pool.free_len += 1;
    }

Here is a minimal reproduction of the error with tests for the memory leak and the panic.
connection_error_repro.zip

Expected Behavior

Presumably repeatedly making and then deinitializing http requests should not be causing either memory leaks or panics due to hitting unreachable code.

@ghost ghost added the bug Observed behavior contradicts documented or intended behavior label Jul 1, 2023
@andrewrk andrewrk added the standard library This issue involves writing Zig code for the standard library. label Jul 7, 2023
@andrewrk andrewrk added this to the 0.11.0 milestone Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

1 participant