Custom thread class leaks resources on Windows

[eric@adblockplus.org]

Background

libadblockplus is using a custom thread class for threading, alternating between native Windows threads and pthread, for Android. This was done because of the immaturity of the development environments at the time it was originally written. In the Windows implementation, the thread constructor system-calls CreateThread and discards the thread handle it returns. As a result, it's impossible to call CloseHandle and recover thread resources. In particular, the thread stack, allocated with process virtual memory, leaks with each thread created.

From MSDN:

The number of threads a process can create is limited by the available virtual memory. By default, every thread has one megabyte of stack space.
[...]
The thread object remains in the system until the thread has terminated and all handles to it have been closed through a call to CloseHandle.

What to change

We now have C++11 threads available and it's time to clear out the legacy code. There's no good reason to improve the old facility, as it offered nothing not already in the standard library.

The solution is to eliminate files Thread.h and Thread.cpp, replacing all they provide with standard C++11 facilities.

[sergz]

Actually, I think it's not too important to simply replace our Thread by std::thread. I think removing of Thread can be done after #3595 because it won't be used anymore.

[eric@adblockplus.org]

Replacing the behavior of setTimeout (mentioned in #3595) is going to require a thread around to wake periodically and do something. The last thing we want is to be mixing our old thread classes with new ones. I didn't mark a dependency, but I don't see a better way of making incremental progress than first clearing out the legacy code.

The issue of detaching threads is separate from which threads we use. One step at a time.

[sergz]

Replying to eric@…:

Replacing the behavior of setTimeout (mentioned in #3595) is going to require a thread around to wake periodically and do something.

If we use e.g. libuv then we will likely not use std::thread nor our Thread class. BTW, I have already implementation of timers with only one instance of std::thread and timers in std::priority_queue but I'm not elaborating it because firstly we need to make a decision how it should be.

The last thing we want is to be mixing our old thread classes with new ones. I didn't mark a dependency, but I don't see a better way of making incremental progress than first clearing out the legacy code.

I don't see a big issue if we use our Thread and std::thread in different parts of application. I think it would be a better way if we firstly fix what create problems (e.g. Lazy* classes don't allow to run many tests and detached threads create race conditions) and then just remove Thread as a dead code.

The issue of detaching threads is separate from which threads we use. One step at a time.

Yes, but simply replacing of Thread by std::thread does not give a benefit, anyway that code will be removed after #3595.

[eric@adblockplus.org]

Replying to sergz:

Yes, but simply replacing of Thread by std::thread does not give a benefit, anyway that code will be removed after #3595.

Actually, it does. I realized that the old code was leaking resources. Changed the description to add it.

The changeset I've posted for review separates out multiple issues that had been combined: task definition, scheduling, and memory allocation. These changes will make any subsequent rewrite that much simpler.

Writing temporary code in the process of a rewrite is just part of the territory. It's not wasteful but rather speeds incremental change along.

Replying to eric@…:
If we use e.g. libuv then we will likely not use std::thread nor our Thread class. BTW, I have already implementation of timers with only one instance of std::thread and timers in std::priority_queue but I'm not elaborating it because firstly we need to make a decision how it should be.

I am not convinced that libuv gets us much on balance. The larger issue is that JavaScript is rather hostile to multithreading. Since it's JS code we're running now, we either (1) keep running JS with known, existing behavior or (2) we rewrite it and induce a testing burden to ensure identical behavior between libadblockplus and adblockpluscore.

In either case, the issue of libuv is really beyond the scope of this issue. This issue is to track a small change toward a larger goal.

[eric@adblockplus.org]

I've changed the title of this ticket, designated it as a defect, and rewritten the description, all to focus on what looks like a serious memory leak with the old thread library. The solution doesn't change, which is simply to upgrade. The priority to land a fix for this, however, should be higher than a maintenance rewrite, since this defect could easily lead to long-running processes containing libadblockplus to behave poorly.