C: Part of the code doesn't execute under select()

Question

I have something like this:

#define QUIT_TIME 5
int main(int argc, char **argv) {
        //... SOCKETS STUFF ....
    fdmax = parentfd;


    while (notdone) {

        //Set the timers
        waitd.tv_sec = 1;
        waitd.tv_usec = 0;

        FD_ZERO(&tempreadfds);
        FD_ZERO(&tempwritefds);

        FD_ZERO(&readfds);          /* initialize the read fd set */
        FD_ZERO(&writefds);         /* initialize the write fd set */

        FD_SET(parentfd, &readfds); /* add listener socket fd */
        FD_SET(0, &readfds);        /* add stdin fd (0) */

        tempreadfds = readfds; //make a copy
        tempwritefds = writefds; //make a copy

        if (select(fdmax+1, &tempreadfds, &tempwritefds, (fd_set*) 0, &waitd) < 0) {
            error("ERROR in select");
        }

        for(i = 1; i <= fdmax; i++) {

            if(FD_ISSET(i, &readfds)) {
                if(i == parentfd) {
                //This is a new connection
                childfd = accept(parentfd, (struct sockaddr *) &clientaddr, &clientlen);
                if (childfd < 0)
                    error("ERROR on accept");

                InitializeDataStructures(childfd);

                FD_SET(childfd, &readfds); //add to the master set
                if(childfd > fdmax)
                    fdmax = childfd;
            } else {
                //Existing connection
                if((nBytes = read(i, connections[i].buffer, MAXBUFFER)) <= 0) {
                    if(nBytes == 0) {
                        //Connection closed
                        printf("Socket %d hung up\n", read_write_loop);
                    } else {
                        error("\nReceive error\n");
                    }

                    FD_CLR(i, &readfds);
                } else {
                    //We have some data from the connection
                                            //... Manipulate the buffer
                    //Handle the message
                }
            }
            }

            if(FD_ISSET(i, &writefds)) {
                                .....
                FD_CLR(i, &writefds);
            }

            //Timer checking
            if(connections[i].active) {
                gettimeofday(&TimeNow, NULL);
                timeval_diff(&Interval, &TimeNow, &connections[i].TimeConnected);
                printf("*_*_*__*_*_*__*_*_*_*_* difference is %ld seconds, %ld microseconds\n",
                         Interval.tv_sec,
                         Interval.tv_usec
                        );
                if(Interval.tv_sec >= QUIT_TIME) {
                    printf("Timer elapsed!!\n");
                }
            }

        }
    }

    /* clean up */
    printf("Terminating server.\n");
    close(parentfd);
    return 0;
}

void InitializeDataStructures(int i) {

    clients[i].active = YES;
    clients[i].fd = i;
    //Initialize other members of the structure
}

long long timeval_diff(struct timeval *difference, timeval *end_time, struct timeval *start_time) {
      struct timeval temp_diff;

      if(difference==NULL)
        difference=&temp_diff;

      difference->tv_sec =end_time->tv_sec -start_time->tv_sec ;
      difference->tv_usec=end_time->tv_usec-start_time->tv_usec;

      while(difference->tv_usec<0)
      {
        difference->tv_usec+=1000000;
        difference->tv_sec -=1;
      }

      return 1000000LL*difference->tv_sec + difference->tv_usec;

    }

I was expecting that the "Timer elapsed" line will be printed every at least once (The TimeConnected was initialized into one of the if conditions) during execution, but for some reason, it never prints out. I thought my while loop should keep printing it... Anyone know if I'm messing up somewhere?

EDIT: Actually, I'm using the timer to disconnect the time after a timeout. I just observed that it prints "Timer elapsed" if another client connects to the server. I did pass the final parameter to select but am not sure why it is not having any effect.

Thanks to bdk!! If you're interested in knowing the "silly" bug I had in this code, read the discussion below in detail... It was an obvious mistake that I overlooked... all because of one sentence in the tutorials: "select modifies your original descriptors".

List of changes:

• Notice that a set of FD_ZERO statements were wrongly placed inside the while loop
• FD_ISSET was being passed readfds and writefds instead of tempreadfds and tempwritefds...

WORKING CODE:

#define QUIT_TIME 5
int main(int argc, char **argv) {
        //... SOCKETS STUFF ....
    fdmax = parentfd;


    FD_ZERO(&readfds);          /* initialize the read fd set */
    FD_ZERO(&writefds);         /* initialize the write fd set */

    while (notdone) {

        //Set the timers
        waitd.tv_sec = 1;
        waitd.tv_usec = 0;

        FD_ZERO(&tempreadfds);
        FD_ZERO(&tempwritefds);


        FD_SET(parentfd, &readfds); /* add listener socket fd */
        FD_SET(0, &readfds);        /* add stdin fd (0) */

        tempreadfds = readfds; //make a copy
        tempwritefds = writefds; //make a copy

        if (select(fdmax+1, &tempreadfds, &tempwritefds, (fd_set*) 0, &waitd) < 0) {
            error("ERROR in select");
        }

        for(i = 1; i <= fdmax; i++) {

            if(FD_ISSET(i, &tempreadfds)) {
                if(i == parentfd) {
                //This is a new connection
                childfd = accept(parentfd, (struct sockaddr *) &clientaddr, &clientlen);
                if (childfd < 0)
                    error("ERROR on accept");

                InitializeDataStructures(childfd);

                FD_SET(childfd, &readfds); //add to the master set
                if(childfd > fdmax)
                    fdmax = childfd;
            } else {
                //Existing connection
                if((nBytes = read(i, connections[i].buffer, MAXBUFFER)) <= 0) {
                    if(nBytes == 0) {
                        //Connection closed
                        printf("Socket %d hung up\n", read_write_loop);
                    } else {
                        error("\nReceive error\n");
                    }

                    FD_CLR(i, &readfds);
                } else {
                    //We have some data from the connection
                                            //... Manipulate the buffer
                    //Handle the message
                }
            }
            }

            if(FD_ISSET(i, &tempwritefds)) {
                                .....
                FD_CLR(i, &writefds);
            }

            //Timer checking
            if(connections[i].active) {
                gettimeofday(&TimeNow, NULL);
                timeval_diff(&Interval, &TimeNow, &connections[i].TimeConnected);
                printf("*_*_*__*_*_*__*_*_*_*_* difference is %ld seconds, %ld microseconds\n",
                         Interval.tv_sec,
                         Interval.tv_usec
                        );
                if(Interval.tv_sec >= QUIT_TIME) {
                    printf("Timer elapsed!!\n");
                }
            }

        }
    }

    /* clean up */
    printf("Terminating server.\n");
    close(parentfd);
    return 0;
}

void InitializeDataStructures(int i) {

    clients[i].active = YES;
    clients[i].fd = i;
    //Initialize other members of the structure
}

long long timeval_diff(struct timeval *difference, timeval *end_time, struct timeval *start_time) {
      struct timeval temp_diff;

      if(difference==NULL)
        difference=&temp_diff;

      difference->tv_sec =end_time->tv_sec -start_time->tv_sec ;
      difference->tv_usec=end_time->tv_usec-start_time->tv_usec;

      while(difference->tv_usec<0)
      {
        difference->tv_usec+=1000000;
        difference->tv_sec -=1;
      }

      return 1000000LL*difference->tv_sec + difference->tv_usec;

    }

Answer 1

Take a look at your select loop parameters, they look fishy to me. Mainly in that you are calling select on tempreadfd and tempwritefd, but then when you call FD_ISSET, you pass it readfd and writefd. before calling select, you are using FD_SET to set all the fd's you are interested in. Since these variables aren't being sent to select, the fds that haven't triggered aren't getting masked. Therefore, you are getting 'activity' detected on all your descriptors. There really isn't any activity on that accept descriptor, so it blocks until a new client connects.

Thats my guess at least.

Answer 2

This is a very code specific problem but who knows... maybe it would be of some help to others.. I ended up using a thread to solve the problem (I know... not the best but I'm just going nuts trying to debug this...). Thanks to everyone who had the patience to help me out...

In the above code, instead of testing the timer inside the while loop of main(), I modified it as follows:

if(ThreadSpawned == 0) {
                pthread_create(&thread, NULL, cleanup, (void *) &fdmax);
                ThreadSpawned = 1;
            }

And then the cleanup function is as follows:

void *cleanup(void *arg) {
    //Timer checking
    int i, *fdmax;
    fdmax = (int *) arg;

    while(1) {
        for(i = 1; i <= *fdmax; i++) {
            if(connections[i].active == 1) {
                gettimeofday(&TimeNow, NULL);
                timeval_diff(&Interval, &TimeNow, &clients[i].TimeConnected);
                printf("*_*_*__*_*_*__*_*_*_*_* difference is %ld seconds, %ld microseconds\n",
                        Interval.tv_sec,
                        Interval.tv_usec
                );
                fflush(stdout);
                if((int) Interval.tv_sec >= QUIT_TIME) {
                    printf("Timer elapsed!!\n");
                    fflush(stdout);
                }
            }
        }
        sleep(20);
    }
}

Any cleaner solutions are most welcome. Perhaps, the reason suggested by bdk in one of his answers is the reason the program is blocking at accept but then, if it blocks there, I'll have no way of calculating a timeout... So I ended up using a thread to do this task...

Answer 3

If I'm reading your code right, you enter your while loop, then check to see if the descriptor is in the read set. If it is, then you go to the accept() portion of your if statement. If it is not, then you enter the else portion, where you immediately block on a read. If the socket is active, bot there is no data available, you will block there until data becomes available. It will not drop through to the section where it even checks the timer until it gets either a successful read or an error on the input.

You should only enter the code where you check the sockets if select returns a value greater than zero, then you should check to see if the socket is in the read set before you attempt to read from it.

Normally you build one fdset to check for the sockets you're accepting connections on, and another for the ones you've accepted and are actually reading data on. I suppose you can do it the way you've presented, but I'd suggest you switch from read() to recv() and use the MSG_PEEK flag.