0

Here is the threaded-server code in C. My question is: do we need to set unused thread to NULL? In java, we need to set thread to NULL to let it return to thread pool.

I made the change to Martin Broadhurst's source code (see gray text as comment)

 /* 
 *  A threaded server
 *  by Martin Broadhurst (www.martinbroadhurst.com)
 *  Compile with -pthread
 */

#include <stdio.h>
#include <string.h> /* memset() */
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <netdb.h>
#include <pthread.h>

#define PORT    "32001" /* Port to listen on */
#define BACKLOG     10  /* Passed to listen() */

void *handle(void *pnewsock)
{
    /* send(), recv(), close() */

    return NULL;
}

int main(void)
{
    int sock;
    pthread_t thread;
    struct addrinfo hints, *res;
    int reuseaddr = 1; /* True */

    /* Get the address info */
    memset(&hints, 0, sizeof hints);
    hints.ai_family = AF_INET;
    hints.ai_socktype = SOCK_STREAM;
    if (getaddrinfo(NULL, PORT, &hints, &res) != 0) {
        perror("getaddrinfo");
        return 1;
    }

    /* Create the socket */
    sock = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
    if (sock == -1) {
        perror("socket");
        return 1;
    }

    /* Enable the socket to reuse the address */
    if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &reuseaddr, sizeof(int)) == -1) {
        perror("setsockopt");
        return 1;
    }

    /* Bind to the address */
    if (bind(sock, res->ai_addr, res->ai_addrlen) == -1) {
        perror("bind");
        return 0;
    }

    freeaddrinfo(res);

    /* Listen */
    if (listen(sock, BACKLOG) == -1) {
        perror("listen");
        return 0;
    }

    /* Main loop */
    while (1) {             
        pthread_attr_t *attr;  //<===I added this
        size_t size = sizeof(struct sockaddr_in);
        struct sockaddr_in their_addr;
        int * ptr;   //<===I added this
        ptr = malloc(sizeof(int));       //<===I added this
        ptr = accept(sock, (struct sockaddr*)&their_addr, &size);
        if (newsock == -1) {
            perror("accept");
        }
        else {
               printf("Got a connection from %s on port %d\n",
                inet_ntoa(their_addr.sin_addr), htons(their_addr.sin_port));  
                //I added the following "if" statement
                if (pthread_attr_setdetachstate(attr, PTHREAD_CREATE_DETACHED) != 0){
                    fprintf(stderr, "Failed to set thread detached\n");
                }
                else {
                    //if (pthread_create(&thread, NULL, handle, &newsock) != 0) {
                    if (pthread_create(&thread, attr, handle, ptr) != 0 ) {
                        fprintf(stderr, "Failed to create thread\n");
                     }
                }

        }
    }

    close(sock);

    return 0;
}

==========-============== code is from here: http://martinbroadhurst.com/source/threaded-server.c.html

user2373043
  • 21
  • 6

3 Answers3

3

No. Well, it's not 100% clear what Java construct it is you're thinking of (I bet there's a close method you can call instead of setting it to null and having the GC take care of it), but that's irrelevant because...

  1. pthread_t is an integer (maybe) type, not a pointer, so it can't be set to NULL.
  2. C is not garbage collected, so even if it were a pointer the thread would have no way of knowing or caring that you set it to null.
  3. POSIX threads does not use a thread pool. The pthread_create function actually creates a brand-new OS-level thread, and returning from the handler actually exits it. (Well, not really. It still hangs around until you call pthread_join, since you didn't create it as a detached thread.)

What you should do is create the threads as detached threads, since your code right now is leaking joinable threads.

Also, using &newsock as the argument is dangerous, since it gets destroyed and recreated in every iteration of the main loop. This is a race condition that probably never showed up in the author's testing because under light load the main thread would be waiting for the next accept to return while it is accessed on the worker thread, and on most systems the same space will be used over and over for the variable.

You can use malloc to create a place to store the socket fd in (which you will need to free at the end of your handler function), or, if your platform allows this (most do), just cast the value to a pointer then cast it back out in the handler function.

Random832
  • 37,415
  • 3
  • 44
  • 63
  • You're right. &newsock is dangerous. More than that, it's just plain broken. It needs fixing, but for something this simple (e.g. newsock is int), I'd (void *) newsock and have the thread do a int newsock = (long) ptr instead of going to malloc. I'd only malloc if there were a control struct being filled in – Craig Estey Oct 14 '15 at 03:55
  • @CraigEstey I recommended malloc because I don't know if the ability to do this is guaranteed to be portable - do you have a POSIX reference about round-trip conversion of integers to pointers and back? – Random832 Oct 14 '15 at 03:56
  • It will always work, even for braindamaged MS compilers that use the LP32/LLP64 model. All Unix systems use LP32/LP64. For 32 bit, it's moot. The following is the reference/standard, formalized back in the 90's http://www.unix.org/version2/whatsnew/lp64_wp.html It even will work on LLP64 because we only need the lower 32 bits. For brevity, I could have done int newsock = (int) ptr; And, I'm a reference as I've done this many times over 30+ years. You can always disguise an int inside a pointer--check the table – Craig Estey Oct 14 '15 at 04:21
  • @CraigEstey Just the size doesn't tell the whole story. In the C standard this is implementation-defined behavior. The page you linked is about round-trip from pointer _to_ long and back to pointer, not the other way around. It would certainly be very unusual for it not to work, but as they say, not all the world's a VAX (or even a "VAX64"). I'm coming at this from a [language-lawyer] point of view, not experience on what actually works on common systems over 30 years. – Random832 Oct 14 '15 at 04:23
  • We may be crossing in the mail. Please read the link I sent you. It is the reference for this, not the C spec. For example, the C spec says that a char may or may not be 8 bits. But, POSIX _mandates_ that a char is 8 bits--they got tired of C spec SJW's ;-) – Craig Estey Oct 14 '15 at 04:28
  • Yes, but does POSIX mandate that you can round-trip a value that started as an int through a pointer cast? The link you posted doesn't appear to address that issue. Also see http://stackoverflow.com/questions/7822904/is-it-always-safe-to-convert-an-integer-value-to-void-and-back-again-in-posix – Random832 Oct 14 '15 at 05:13
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/92214/discussion-between-craig-estey-and-random832). – Craig Estey Oct 14 '15 at 05:41
  • @Random832, I made the change to Martin Broadhurst's source code (see gray text as comment). Are they correct? – user2373043 Oct 15 '15 at 01:45
  • @user2373043 no - you're not putting the file descriptor value anywhere now. Do you understand how C and pointers work in general? – Random832 Oct 15 '15 at 03:21
  • @Random832, pthread_create(&thread, attr, handle, ptr), so i put the file descriptor value to the memory address ptr is pointing to. and handle(ptr) will use this file descriptor value to do some work in the new thread. – user2373043 Oct 16 '15 at 00:53
  • @user2373043 At this point you're doing several things that are getting away from the original question. You need `*ptr = [...connect socket...]` if you're going to use malloc in the way I stated, and if you're using a pthread_attr to set detached mode I think you need to do it _before_ creating the thread. – Random832 Oct 16 '15 at 00:55
  • @Random832, I changed the code. I set detached mode before creating the thread. As to *ptr = [...connect socket...], connect() is done in a client code, not in a server code. So I don't know how to add *ptr = [...connect socket...] to this code. – user2373043 Oct 16 '15 at 03:49
  • @user2373043 I was talking about the same code you have under "ptr =" now, right after the malloc. The accept call. Sorry for bad phrasing. – Random832 Oct 16 '15 at 03:51
  • `pthread_t` is not an integer type. It's an opaque, implementation-defined type, and there is no reserved value for "no thread". – R.. GitHub STOP HELPING ICE Oct 16 '15 at 03:51
  • @R.. In the version I read it was required to be an arithmetic type, I didn't realize I had an old version open. – Random832 Oct 16 '15 at 03:53
2

Since C doesn't have objects, there is no object that represents the thread and so nothing to set to NULL. A detached thread will go away when it terminates. An undetached thread will go away when it's joined.

David Schwartz
  • 179,497
  • 17
  • 214
  • 278
0

You have a pthread_t as thread id, there is no object, and it don't work in the way as java gc.

Thread terminate in 1 of following cases:

  • its start function return,
  • the thread call pthread_exit()
  • canceled by pthread_cancel()
  • any of threads in the process call exit(),
  • the main thread returns, in this case, all threads in the process will terminate immediately,
Eric
  • 22,183
  • 20
  • 145
  • 196