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

Detached threads don't get disposed of #8201

Closed
VirtualTim opened this issue Feb 27, 2019 · 11 comments · Fixed by #8286
Closed

Detached threads don't get disposed of #8201

VirtualTim opened this issue Feb 27, 2019 · 11 comments · Fixed by #8286

Comments

@VirtualTim
Copy link
Collaborator

According to this "Any allocated resources will be freed once the thread exits." However when a detached thread exits it doesn't get disposed. So not only does that leak memory, it makes maintaining a thread pool not really work.

Example code (swap 100 with something like 10 it you don't want to go out of memory):

#include <thread>
#include <iostream>

void some_function() {
	std::cout << "Thread ID: " << std::this_thread::get_id() << std::endl;
}

int main(int argc, char** argv) {
	for (int i=0; i<100; i++) {
		auto t1 = std::thread(some_function);
		t1.detach();
	}
}

Compile like:
emcc test.cpp -std=c++11 -s USE_PTHREADS=1 -s TOTAL_MEMORY=52428800 -o test.html

With the hardocded 100 threads I go out of memory at about 20. Changing the hardcoded value to 10 doesn't go OOM, but the workers still never get freed.

@kripken
Copy link
Member

kripken commented Feb 27, 2019

Is this a browser OOM? We don't destroy workers, I believe, so that would make sense. If we destroyed them, we'd have issues with their creation (it's async, that's why we have the compile option to spin them up beforehand). So not sure we can do much better here. At minimum we should document this better though.

@VirtualTim
Copy link
Collaborator Author

VirtualTim commented Feb 28, 2019

Not browser OOM, the program just reaches the TOTAL_MEMORY limit. I don't think pthreads work with ALLOW_MEMORY_GROWTH, but each worker takes a decent amount of memory, so if this is ever supported there will be issues, I'd imagine.
Edit: actual error: Cannot enlarge memory arrays to size 52428800 bytes (OOM). Either (1) compile with -s TOTAL_MEMORY=X with X higher than the current value 52428800, (2) compile with -s ALLOW_MEMORY_GROWTH=1 which allows increasing the size at runtime, or (3) if you want malloc to return NULL (0) instead of this abort, compile with -s ABORTING_MALLOC=0

I would have thought that if a detached thread gets disposed, either the webworker would get disposed, or it would just go back into the thread pool. As it is worker just keep getting created until
TOTAL_MEMORY is reached.

For what I was currently working on, I was registering a click callback on the canvas element, and on click create a thread that performs some lengthy synchronous operation (related to the fun I was having with #8117). Obviously joining the thread would block the main thread (or in my case create a deadlock). However if every click creates a web worker that never gets disposed the application is going to quickly run out of memory.

If we destroyed them, we'd have issues with their creation

Can you explain further what the issue is here?

@kripken
Copy link
Member

kripken commented Feb 28, 2019

I see. Yeah, we intend to have memory growth working with pthreads, but this is not yet ready.

If we destroyed them, we'd have issues with their creation

I meant the same issues at startup - when you do pthread_create, if we need to spin up a worker, that's an async operation, so it must return to the main browser event loop for the thread to actually be created. To work around that, we can preload some workers at startup, and then pthread_create just activates an existing worker, which we can do synchronously.

@VirtualTim
Copy link
Collaborator Author

Would there be a way to return detached threads to the thread pool when they finish?
So for example you could call:

std::thread(some_function).detach();     //running on thread 1
sleep(5000)    //give the thread time to finish (pretend this doesn't block the main thread)
//thread 1 is finished and returned to the pool
std::thread(another_function).detach();  //another_function gets run on thread 1

and both functions could run on the same thread?

@kripken
Copy link
Member

kripken commented Mar 4, 2019

I believe that's how things currently work, yes, there is a pool that is reused. (Assuming .detach() in fact uses pthread_exit or pthread_kill or such to halt the child thread?)

@VirtualTim
Copy link
Collaborator Author

Hm, that doesn't seem to be the case in my testing. I'm guessing the example I created above creates threads too fast, but in another project I'm creating a thread per mouse click (using a callback attached to the canvas) and it seems like that created additional threads. I may try making another example to better show this.

@kripken
Copy link
Member

kripken commented Mar 7, 2019

For debugging this, I think the key location is unusedWorkerPool in src/library_pthread.js.

Looks like the cancelDone event should lead to a worker being cleaned up and re-added to that list. Adding some debug prints may help see why it's not happening in your case.

@VirtualTim
Copy link
Collaborator Author

Thanks for the pointer. I looks like the d.cmd === 'exit' is a noop?
When I changed } else if (d.cmd === 'cancelDone') { to } else if (d.cmd === 'cancelDone' || d.cmd === 'exit') {, and commented out the line } else if (d.cmd === 'exit') { I get the expected behaviour.
Are there any obvious issues you see here? Should I create a PR and see if the tests pass?

The initial example I posted still creates threads too fast, and threads aren't removed from the pool (which seems reasonable). However an example like this behaves as expected, with detached threads being returned to the pool.

void click_callback_thread(const EmscriptenMouseEvent* e) {
	std::cout << "Thread ID: " << std::this_thread::get_id() << std::endl;
}

EMSCRIPTEN_RESULT click_callback(int eventType, const EmscriptenMouseEvent* e, void* userdata) {
	std::thread t1(click_callback_thread, e);
	t1.detach();
	return EMSCRIPTEN_RESULT_SUCCESS;
}

int main(int argc, char** argv) {
	emscripten_set_mousedown_callback(nullptr, 0, EM_TRUE, click_callback);
}

@benjymous
Copy link

benjymous commented Mar 8, 2019

I'm seeing the same issue using std::async - an event being triggered launches a background thread (if one isn't already being run) - what I see in emscripten compiled code is that a new thread appears each time std::async is called, which, as above rapidly causes memory to run out - there will only ever be one async-launched thread running at a time.

std::async documentation suggests that the implementation likely uses a threadpool under the hood, but there is no user facing configuration for this in the specification

@VirtualTim
Copy link
Collaborator Author

Does the fix I mentioned above also fix your std::async issue?

@kripken
Copy link
Member

kripken commented Mar 11, 2019

@VirtualTim - yeah, I'd suggest opening a PR. I'm not familiar enough with that code to say offhand. Can discuss in a PR and hopefully other people can chime in there too.

kripken pushed a commit that referenced this issue May 20, 2019
Once detached threads are finished their execution they emit the 'exit' command. Instead of a noop they should rejoin the pool.

Resolves #8201.
VirtualTim added a commit to VirtualTim/emscripten that referenced this issue May 21, 2019
Once detached threads are finished their execution they emit the 'exit' command. Instead of a noop they should rejoin the pool.

Resolves emscripten-core#8201.
belraquib pushed a commit to belraquib/emscripten that referenced this issue Dec 23, 2020
Once detached threads are finished their execution they emit the 'exit' command. Instead of a noop they should rejoin the pool.

Resolves emscripten-core#8201.
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 a pull request may close this issue.

3 participants