Skip to content

Conversation

StefanoPetrilli
Copy link
Contributor

The current code was using std::vector<crow::websocket::connection*> websockets_; to keep track of the websockets created in the App class. The websockets_ is cleaned when the App.close() is called.
On the other hand, the function to populate the vector (App::add_websocket), was never being called. This means that in practice it was failing to keep track of the websockets connection and therefore to clean them.

The use of raw pointers and the logical error which resulted in not correctly updating websockets_ was resulting in the memory leak reported in #986. Whenever the App is closed before a connection had time to terminate or in the case delete this was not being called on the wesocket::Connection.

This PR solves the memory leak by executing add_websocket whenever a websocket connection is created and migrates from the use of raw pointers and manual disposal to shared_pointers.

This PR might also solve all the other known instances of memory leak in the websocket implementation (#700 and #529) but I had no time to verify it.

@StefanoPetrilli
Copy link
Contributor Author

Replicating the memory leak was a bit convoluted, what I did is:

I had this SimpleApp (similar to the one proposed in from #986):

#include <chrono>
#include <crow.h>
#include <thread>
#include <iostream>
#include <chrono>
#include <crow.h>
#include <thread>

int main()
{
    crow::SimpleApp app;

    CROW_WEBSOCKET_ROUTE(app, "/ws")
        .onopen([&](crow::websocket::connection &conn) {
            CROW_LOG_INFO << "new websocket connection from " << conn.get_remote_ip();
        })

        .onclose([&](crow::websocket::connection &, const std::string &reason, uint16_t) {
            CROW_LOG_INFO << "websocket connection closed: " << reason;
        })

        .onmessage([&](crow::websocket::connection &conn, const std::string &data, bool is_binary)
        {
            CROW_LOG_INFO << "Recv message:" << data;
            if (is_binary)
                conn.send_binary(data);
            else
                conn.send_text(data);
            // Stop the app after receiving one message so sanitizers can run and report leaks.
            app.stop();
        });

    auto worker = [&]() {
        app.port(18080).run();
    };

    std::thread worker_thread(worker);
        // Wait for user input so we have time to connect a websocket client.
        std::cout << "Server running on port 18080. Press Enter to stop...\n";
        std::cin.get();
    app.stop();
    if (worker_thread.joinable()) {
        worker_thread.join();
    }
}

I executed it in one terminal and then run this python script in another terminal to open a websocket connection

import time
import websocket

def main():
    url = "ws://127.0.0.1:18080/ws"
    message = "hello"
    retries = 5
    delay = 0.2
    hold = 0.5

    ws = None
    for attempt in range(retries):
        try:
            ws = websocket.create_connection(url, timeout=3)
            print(f"connected to {url}")
            break
        except Exception as e:
            print(f"attempt {attempt} failed: {e}")
            time.sleep(delay)
    if not ws:
        raise SystemExit("Could not connect to server")

    try:
        ws.send(message)
        print(f"sent: {message}")
        try:
            msg = ws.recv()
            print(f"received: {msg}")
        except Exception as e:
            print(f"recv error (may be normal): {e}")
        time.sleep(hold)
    finally:
        try:
            ws.close()
        except Exception:
            pass
        print("connection closed")


if __name__ == '__main__':
    main()

The app stopping triggers the memory leak which I detected using asan

close_handler_(std::move(close_handler)),
error_handler_(std::move(error_handler)),
accept_handler_(std::move(accept_handler))
static void create(const crow::request& req, Adaptor adaptor, Handler* handler,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoudn't std::make_shared(...) do the same as this function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants