Member Avatar for Agni

This code here is giving segmentation fault. Any clues why? also any general tips for writing good multi-threaded code are welcome.

#include <windows.h>
#include <process.h>
#include <iostream>

using namespace std;

unsigned __stdcall Display(void* p)
{
	Sleep(500);
	cout << "Display" << endl;
	cout << *((int*)p) << endl;
	return 0;
}
int main()
{
	unsigned taddr = NULL;
	unsigned createFlag = 0;

	for(int i=0;i<5;i++)
	{
		if (_beginthreadex(NULL,0,Display,&i,createFlag,&taddr))
		{
			cout << "success" << endl;
		}
		else
			cout << "failed" << endl;
	}

	cout << "main's execution is over" << endl;
	ExitThread(0);
}
  • 4 Contributors
  • 6 Replies
  • 199 Views
  • 13 Hours Discussion Span
  • Latest Post Latest Post by Salem

Dani AI

Generated

The crash is caused by passing the address of the loop variable into each thread: every thread receives the same pointer to i, which changes as the loop runs and disappears when main ends. identified the lifetime issue correctly. ’s idea to keep the creator alive (so threads finish) is also right; ’s comment needs the qualification that returning from main (or calling ExitProcess/exit) terminates the whole process and all threads, while ExitThread only terminates the calling thread and can bypass CRT cleanup — avoid using it from main.

A modern, simple fix is to pass the value by copy (no shared pointer to the loop variable). For example, using C++11 threads:

#include <thread>
#include <vector>
#include <iostream>
#include <chrono>

int main() {
    std::vector<std::thread> threads;
    for (int i = 0; i < 5; ++i) {
        threads.emplace_back([i]() {
            std::this_thread::sleep_for(std::chrono::milliseconds(500));
            std::cout << "Display\n" << i << std::endl;
        });
    }
    for (auto &t : threads) t.join();
}

If you must stick with _beginthreadex, give each thread its own stable storage: allocate an int per thread (and have the thread free it), or pre-allocate a std::vector<int> and pass pointers to distinct elements. Important cautions: don’t cast int directly to void* on 64-bit systems (use intptr_t), always wait/join threads instead of killing them, and call CloseHandle for handles returned by _beginthreadex.

Other practical tips: protect shared resources (including std::cout) with a mutex, keep thread work small and composable, and debug with small reproductions (remove Sleep, log thread ids/addresses, run under a debugger to catch access violations).

Recommended Answers

All 6 Replies

Member Avatar for Ancient Dragon

>>ExitThread(0);
delete that line in main()

Member Avatar for Agni

>>ExitThread(0);
delete that line in main()

But if i do that the threads never execute as 'main' terminates and all the threads die. Any reason why you think it might help?

I found a possible solution to the problem, since the threads would sleep in 'Display' and by the time they wake up, main exits hence 'i' doesn't exist anymore and when i try to print the value it crashes. Actually i executed it once without the 'sleep' and it went through fine. Though i think it might still crash some other time when main exits before any of the threads is left incomplete.

Member Avatar for unbeatable0

It doesn't matter that 'i' doesn't exist when main() ends and the thread is using it, simply because when main() ends the whole program shuts down, including the threads.

Member Avatar for Agni

It doesn't matter than 'i' doesn't exist when main() ends and the thread is using it, simply because when main() ends the whole program shuts down, including the threads.

The threads wont shut down because I'm using 'ExitThread', if I'm not wrong its the equivalent of pthread_exit for Unix.

Member Avatar for Ancient Dragon

ExitThread() belongs inside a thread, not in main().

ExitThread is the preferred method of exiting a thread in C code.

There are other methods available to keep main() alive until the threads terminate on their own.

#include <windows.h>
#include <process.h>
#include <iostream>

using namespace std;
unsigned __stdcall Display(void* p)
{
	Sleep(500);
	cout << "Display" << endl;
	cout << *((int*)p) << endl;
	return 0;
}
int main()
{
	unsigned taddr = NULL;
	unsigned createFlag = 0;
    HANDLE handles[5] = {0};
 	for(int i=0;i<5;i++)
	{
		if (handles[i] = (HANDLE)_beginthreadex(NULL,0,Display,&i,createFlag,&taddr))
		{
			cout << "success" << endl;
		}
		else
			cout << "failed" << endl;
	}
    WaitForMultipleObjects(5, handles, TRUE, INFINITE);

	cout << "main's execution is over" << endl;
}
commented: Thanks +2
Member Avatar for Salem

> Any clues why?
Because you passed a POINTER to i.
The main() thread exits, the i variable goes out of scope and another thread is looking down the barrel of a segfault.

> also any general tips for writing good multi-threaded code are welcome.
Well the first step is to make sure that whatever parameter you pass has a lifetime which exceeds the thread creator.

Using dynamic memory, and then making the thread the owner of that memory is one way to achieve this.
This also solves the other problem of helping to ensure that only one thread has access to the memory passed into it.

commented: Thanks for the tip ... +2
Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.