Skip to content

Library leaks sockets when it cannot connect to the database #208

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

Closed
Akvinikym opened this issue Aug 11, 2022 · 0 comments · Fixed by #228
Closed

Library leaks sockets when it cannot connect to the database #208

Akvinikym opened this issue Aug 11, 2022 · 0 comments · Fixed by #228
Labels
bug Something isn't working priority-high Pretty important bug or issue

Comments

@Akvinikym
Copy link

Hello.

I have a minimal example of the (I assume) correct usage of the library which leads to sockets being leaked by it. Steps to reproduce:

  1. Get the clickhouse-server up
  2. Run this program:
#include <clickhouse/client.h>
#include <thread>

int main()
{
	clickhouse::Client client(clickhouse::ClientOptions().SetHost("localhost").SetPort(9000).SetUser("default").SetPassword("clickhouse"));
	client.Execute("CREATE DATABASE IF NOT EXISTS test");
	client.Execute("CREATE TABLE IF NOT EXISTS test.numbers (id UInt64) ENGINE = Memory");

	bool error_happened = false;
	uint64_t i = 0;
	while (true)
	{
		std::this_thread::sleep_for(std::chrono::seconds(1));

		try
		{
			if (error_happened)
			{
				// doing it here and not in catch block because if ResetConnection() fails,
				// it will throw another exception, leading to program finish
				client.ResetConnection();
				error_happened = false;
			}

			clickhouse::Block block;
			auto id_column = std::make_shared<clickhouse::ColumnUInt64>();
			id_column->Append(i);
			block.AppendColumn("id", id_column);
			client.Insert("test.numbers", block);
		}
		catch (const std::exception& e)
		{
			printf("exception: %s\n", e.what());
			error_happened = true;
		}

		++i;
	}
}
  1. Take a look at the number of sockets used by the program (it will be equal to 1), for example, by executing the command:
ls -la /proc/${PID}/fd | grep socket | wc -l
  1. Stop the clickhouse-server
  2. Execute the command from step 3 to see number of socket files rising up to infinity (or the OS hard limit)
  3. Get the clickhouse-server up again
  4. Program will successfully reconnect and continue sending data, but the number of used sockets will remain at the same level

I think this issue happens because in function SocketConnect(..) the socket is created on line 126, but if connect(..) on line 134 fails, close(..) is never called for this socket object, which is required according to the documentation (see NOTES section).

If I add close(..) calls before lines 149 and 166, the program stops leaking sockets and is executed successfully. I could have made a PR for those changes, but I'm not sure they're correct or enough to cover all cases.

@Enmk Enmk added bug Something isn't working priority-high Pretty important bug or issue labels Oct 4, 2022
@Enmk Enmk closed this as completed in #228 Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-high Pretty important bug or issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants