I'm trying to build a basic "game" server in C, I stock each file descriptor of an accepted socket in an int *, each of these are stocked in int ** clients. I then create a thread and pass clients as its argument.
Here's the code:
game_server_threaded.c
#define PLAYERS 4
int ** clients;
/* Setting up server... */
clients = malloc(sizeof(int *) * PLAYERS);
while (1)
{
for (int i = 0; i < PLAYERS; i ) {
clients[i] = malloc(sizeof(int));
*clients[i] = accept(server_sock,
(struct sockaddr *)&client_address, &size);
printf("s%d: %d\n", i, *clients[i]);
}
pthread_t th;
if (pthread_create(&th, NULL, play_game, clients) < 0)
die("create thread");
}
void * play_game(void * s)
void * play_game(void * s)
{
int ** clients = (int **) s;
srand (time(NULL));
int k = rand() % MAX_K;
int tries = 3;
int turn = 0;
for (int i = 0; i < PLAYERS; i )
{
printf("s%d: %d\n", i, *clients[i]);
}
/* Game mechanics... */
}
However, the output is very weird. In the thread, all *clients[i] are set, except for *clients[0].
Here it is:
s0: 4
s1: 5
s2: 6
s3: 7
s0: 0
s1: 5
s2: 6
s3: 7
I've tested this extensively with different casts and such. I've also created a version which sends this struct to the thread:
struct player_threads {
char * msg;
char c;
int ** clients;
};
In the thread, msg and c are intact but the first element of clients is still 0!
I managed to get it working with a simple int * clients, but still, I can't comprehend this behaviour, could someone explain it to me?
Thank you.
EDIT****
The file from which the above code comes from is relatively short, it can be found here if you want to reproduce it.
CodePudding user response：
The created threads are sharing data that they expected to remain stable but is actually being modified outside the thread.
Specifically, the memory pointed to by clients, pointing to an "array" of int * is allocated before any threads are created. It is being filled in with a new set of int * before each new thread is created and is then passed to the new thread in the final argument of the pthread_create call. When the new thread examines the "array" of int *, it may see a mixture of the original elements at the time of the pthread_create call plus some elements that have since been overwritten by the main thread. Also, because assignment to an object of pointer type is not guaranteed to be atomic, it is also possible for the thread to access a partially updated (and therefore invalid) pointer value.
In the example given, the first four lines are printed by the main thread by this code:
for (int i = 0; i < PLAYERS; i ) {
clients[i] = malloc(sizeof(int));
*clients[i] = accept(server_sock,
(struct sockaddr *)&client_address, &size);
printf("s%d: %d\n", i, *clients[i]);
}
Output:
s0: 4
s1: 5
s2: 6
s3: 7
The main thread then creates a new thread:
pthread_t th;
if (pthread_create(&th, NULL, play_game, clients) < 0)
die("create thread");
The main thread then repeats the above for loop. The first thing that does is overwrite clients[0] with a new pointer value from malloc(sizeof(int)) and then it blocks on the accept call.
Meanwhile, the thread starts and prints stuff from the clients array, but clients[0] may have already been overwritten.
Output:
(At this point, the main thread has overwritten client[0]. The contents of client[0][0] are indeterminate at this point, but happens to contain 0 here.) ...
s0: 0
(At this point, the main thread is blocked on the call to accept, so clients[1] through to clients[3] still have the same values they had at the time of the call to pcreate_thread.) ...
s1: 5
s2: 6
s3: 7
To correct the problem, a new clients block needs to be allocated for each thread. That can be done by moving the allocation of clients into the loop that creates the new threads:
#define PLAYERS 4
int ** clients;
/* Setting up server... */
while (1)
{
clients = malloc(sizeof(int *) * PLAYERS);
for (int i = 0; i < PLAYERS; i ) {
clients[i] = malloc(sizeof(int));
*clients[i] = accept(server_sock,
(struct sockaddr *)&client_address, &size);
printf("s%d: %d\n", i, *clients[i]);
}
pthread_t th;
if (pthread_create(&th, NULL, play_game, clients) < 0)
die("create thread");
}
As an aside, there is unnecessary indirection in the use of int **clients since all that is really being passed in this example is one int value per player (containing the file descriptor of the accepted socket connection for that player). That could be simplified as follows:
Main thread
#define PLAYERS 4
int * clients;
/* Setting up server... */
while (1)
{
clients = malloc(sizeof(int) * PLAYERS);
for (int i = 0; i < PLAYERS; i ) {
clients[i] = accept(server_sock,
(struct sockaddr *)&client_address, &size);
printf("s%d: %d\n", i, clients[i]);
}
pthread_t th;
if (pthread_create(&th, NULL, play_game, clients) < 0)
die("create thread");
}
Game thread
void * play_game(void * s)
{
int * clients = s;
srand (time(NULL));
int k = rand() % MAX_K;
int tries = 3;
int turn = 0;
for (int i = 0; i < PLAYERS; i )
{
printf("s%d: %d\n", i, clients[i]);
}
/* Game mechanics... */
}
For the version that passes a pointer to a struct player_threads to each thread, with the clients member type simplified to int *:
struct player_threads
struct player_threads {
char * msg;
char c;
int * clients;
};
Main thread
#define PLAYERS 4
struct player_threads * players;
/* Setting up server... */
while (1)
{
players = malloc(sizeof(*players));
players->clients = malloc(sizeof(int) * PLAYERS);
players->msg = "Hello";
players->c = 'X';
for (int i = 0; i < PLAYERS; i ) {
players->clients[i] = accept(server_sock,
(struct sockaddr *)&client_address, &size);
printf("s%d: %d\n", i, players->clients[i]);
}
pthread_t th;
if (pthread_create(&th, NULL, play_game, players) < 0)
die("create thread");
}
Game thread
void * play_game(void * s)
{
struct player_threads * players = s;
srand (time(NULL));
int k = rand() % MAX_K;
int tries = 3;
int turn = 0;
for (int i = 0; i < PLAYERS; i )
{
printf("s%d: %d\n", i, players->clients[i]);
}
/* Game mechanics... */
}
As another aside, it would be better to replace the calls to srand and rand with a call to rand_r because the rand call might not be thread-safe:
unsigned int seed = time(NULL);
int k = rand_r(&seed) % MAX_K;
seed needs to be in storage local to the thread. In the above, it is on the thread's stack, but it may be better to provide storage for it as a member of struct player_threads:
struct player_threads {
char * msg;
char c;
unsigned int seed;
int * clients;
};
players->seed = time(NULL);
int k = rand_r(&players->seed) % MAX_K;