File locking between server threads

Question

I have written the below code where the server is getting requests from clients on a socket and creating a thread per client. Each client thread is then writing to a file which is common to all the threads. The file has been opened when main starts, so the same fd is being used by each thread. In such a scenario I have tried to achieve locking on file when one thread is writing to it. Since the threads are of same process so flock can't simply lock the file, hence mutex is used.

/*Logging function*/
void write_to_file(int op_fd,char *name)
{
 flock(op_fd,LOCK_EX);
 pthread_mutex_lock(&f_lck);
 write(op_fd,name,20);
 pthread_mutex_unlock(&f_lck);
 flock(op_fd,LOCK_UN);
}
/*Client thread function*/
void *clnt_thread(void * arg)
{
 int this_fd=*(int *)arg;
 /*str is a continuous memory array of the size of the dat_com struct.This will be filled up initially on receive as a normal array
 , but then it'll typecasted to point to struct dat_com object*/
 char str[sizeof(dat_com)],str_send[25];
 memset(&str,'0円',sizeof(str));
 dat_com *incm_dat; // struct to contain the incoming data from client .
 while(1)
 {
 read(this_fd,str,sizeof(str));
 /*typecast to struct dat_com so that it is ready to be inserted into linked list or file.*/
 incm_dat=(dat_com *)&str;
 if(strncmp(str,"quit",4)==0)
 break;
 /*Call write to file function*/
 write_to_file(o_fd,incm_dat->name);
 fprintf(stderr,"Client said: %s",incm_dat->name);
 /*client wants to close connection?*/
 sprintf(str_send,"%s",incm_dat->name);
 send(this_fd,str_send,sizeof(incm_dat->name),MSG_NOSIGNAL);
 }
close(this_fd);
return 0;
}

This is working as expected at the moment. Is it good practice to lock this way, or is there any other best practice? If I have to put this code into production, what changes should I make that are not standard practice?

Answer 1

There is no reason to use flock and a mutex, unless you are sharing with another process (as opposed to thread). I would just use the mutex and remember to check the return values from the pthread_* and write calls.

Some other comments:

• name in write_to_file should be const and the write call in that function should use the length of the name, not an assumed 20.

• you need to check the status of function calls (read, write, send etc).

• depending upon where the data being written is coming from and what it contains, you may have byte-ordering problems that are not handled.

• Since you are mixing string data and dat_com structures on the socket, you risk getting them mixed up. You'd be better sticking with one format, maybe strings and have a way to check alignment and log/recover-from errors.

• Your fprint calls are not synchronized across threads and may end up interleaved.

• When sending the name, there is no need to copy it into a string first.

  sprintf(str_send,"%s",incm_dat->name);
  send(this_fd,str_send,sizeof(incm_dat->name),MSG_NOSIGNAL);
  

  just send it directly:

  send(this_fd, incm_dat->name, strlen(incm_dat->name), MSG_NOSIGNAL);
  

• Your variable and type names are poorly chosen. dat_com means little, this_fd should be just fd, incm_dat could be just data, o_fd as a global should be longer and more descriptive of function.

• Diwakar Sharma: Thanks for suggestions ! I didn't get your fourth point(..mixing string data and dat_com structures..). Bcz what I have known is we can't directly read a structure from socket, hence I read same amount of bytes and then type casted.

Answer 2

I would introduce a layer in between your threads and the file.

E.g. have a thread that handles all writing to the file, it holds a mutex protected queue which is written by the other two threads. That way you have a layer between the physical file (slow access) and the queue (fast access). You then do not have to lock the file in any way since only the thread that writes the file needs to have access to it.

---