Why is socket not working when multithreaded?

Question

I have a very simple recvfrom() command that works fine - so long as it is not called in "another" thread.

I would post more code, but there is quite a bit of it, so hopefully I can filter out the relevant bits: First we have the global variable: SOCKET Socket=socket(AF_INET,SOCK_DGRAM,IPPROTO_UDP);.

So long as threads are not involved, this works fine:

char message[_max_message_];
struct sockaddr_in* from;
int r;
    int SenderAddrSize = sizeof (struct sockaddr);
    r=recvfrom(Socket,message,_max_message_,0,(struct sockaddr *)&from,&SenderAddrSize);
    printf("Bytes recieved: %i\nError Code: %i\n",r,WSAGetLastError);

Now I have identical code called behind a thread, like this: pthread_create(&listener, NULL, listenloop, &Socket);

(The code basically ignores &socket.)

The first recvfrom() to execute, from the called thread, returns -1, but the recvfrom() from the "original" thread (where the networking was setup) successfully fills message with the, well, message from the server.

So kind as to tell me what I'm doing wrong?

EDIT: I hate to throw more than a dozen lines at strangers kind enough to help me, but I don't think I'm gonna get an answer if I don't. So, here is the kit and kaboodle, edited slightly:

#include <iostream>
//#include <stdio.h>
#include <stdlib.h>
#include <stdio.h>

#include <pthread.h>
#include <conio.h>

using namespace std;
#include <string>
//One thread shall listen continually for responses from the server.
/*The other thread shall listen continually for user input, and fire off user input at the local
 client to the server...*/

//#ifdef _WINDOWS
#include <winsock2.h>
#include <ws2tcpip.h>
#include <windows.h>

SOCKET Socket = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
inline int randport()
{
  return (50000 % rand() + 1000);
}
#define _serverip_ "***.***.***.***"
#define _welcome_ "Welcome,Wagon!"

#define _randomport_ 64000%rand()+100
#define _max_message_ 100

void *listenloop(void *arg)
{
  //SOCKET* listener = (SOCKET)arg;
  WSADATA WsaDat;
  WSAStartup(MAKEWORD(2, 0), &WsaDat);

  char message[_max_message_];
  //SOCKET listener=(SOCKET)arg;
  int r;
  //sockaddr_in SenderAddr;
  struct sockaddr_in from;
  //while (1){

  int SenderAddrSize = sizeof(struct sockaddr);
  r = recvfrom(Socket, message, _max_message_, 0, (struct sockaddr *) &from,
      &SenderAddrSize);
  printf("Thread Bytes recieved: %i\nThread Error Code: %i\n", r,
      WSAGetLastError);
  return NULL ;

  //}
  return NULL ;
}

int main()
{
  string user, pass, login;
  WSADATA WsaDat;
  WSAStartup(MAKEWORD(2, 0), &WsaDat);
  int port;
  cout << "Welcome!"
  SOCKET Socket = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);

  fflush(stdin); //As long as we compile with GCC Behavoir should be consistant

  //TRY NOT TO SEND PLAINTEXT PASSWORDS LIKE THIS!  IT MAY MAKE YOUR USERS VULNERABLE!  DONE FOR SAKE OF SIMPLICITY HERE!

  cout << "\n\nPlease enter the username you registered with:";
  getline(cin, user);
  cout << "\nPlease enter your password, my good sir: ";
  getline(cin, pass);
  struct hostent *host;
  host = gethostbyaddr(_serverip_, strlen(_serverip_), AF_INET);

  if (host == NULL )
  {
    cout << "\n\n UNABLE TO CONNECT TO SERVER.  QUITTING. ";
    return -1;
  }

  short errorcount = 3;
  int socketfeedback;

  ///Put the address for the server on the "evelope"

  SOCKADDR_IN SockAddr;
  SockAddr.sin_port = htons(port);
  SockAddr.sin_family = AF_INET;
  SockAddr.sin_addr.s_addr = inet_addr(_serverip_);

  ///Sign the letter...

  int myport = _randomport_;
  int code;

  SOCKADDR_IN service;
  service.sin_family = AF_INET;
  service.sin_addr.s_addr = inet_addr("localhost");
  service.sin_port = htons(myport);

  //bind(Socket, (SOCKADDR *) &service, sizeof(service));

  //Start a thread, listening for that server

  while ((errorcount))
  {
    code = bind(Socket, (SOCKADDR *) &service, sizeof(service));
    if (code)
      break;
    else
      return -5;
    errorcount--;
    myport = _randomport_;
    service.sin_port = htons(myport);
  }

  login = user + ',' + pass;

  if (!errorcount)
  {
    cout << "\n\nMiserable failure.  Last Known Error Code: " << code;
    return -1;
  }

  ///Begin the listen loop!!

  pthread_t listener;
  pthread_create(&listener, NULL, listenloop, &Socket);
  struct sockaddr result;
  sendto(Socket, login.c_str(), strlen(login.c_str()), 0,
      (struct sockaddr *) &SockAddr, sizeof(SockAddr));

  char message[_max_message_];
  //SOCKET listener=(SOCKET)arg;

  //sockaddr_in SenderAddr;
  struct sockaddr_in from;
  int r;
  int SenderAddrSize = sizeof(struct sockaddr);
  r = recvfrom(Socket, message, _max_message_, 0, (struct sockaddr *) &from,
      &SenderAddrSize);
  printf("Bytes recieved: %i\nError Code: %i\n", r, WSAGetLastError);

  //SOCKET listener=(SOCKET)arg;

  WSACleanup();

  return 0;

}

Answer 1

Why do you use global Socket? And why are you declaring another Socket in main? You should better use the socket passed in pthread_create (just cast args in listenloop to SOCKET *). Global variables in multithreaded are a really bad idea (you need synchronization mechanism). And initialize your struct sockaddr_in from with zeros (for e.g. with memset, or just do as alk said : struct sockaddr_in from = {0}).

And also you are reading from one socket in two different threads without any kind of synchronization. This is bound to cause many errors.

And also I see a problem with WSACleanup and recvfrom in other thread. You don't know in what order will these two run (so you can also get WSACleanup before you can recvfrom in other thread).You can use pthread_join to wait for other thread to finish and then do WSACleanup.

Answer 2

This is too long for a comment.

The code as posted would not work at all, due to declaring:

struct sockaddr_in* from;

and then using from like this:

r=recvfrom(Socket,message,_max_message_,0,(struct sockaddr *)&from,&SenderAddrSize);

You are paasing the address of the address of struct sockaddr_in instead of only its address.

Is shall be:

r=recvfrom(Socket,message,_max_message_,0,(struct sockaddr *)from,&SenderAddrSize);

However if doing so you are missing to allocate memory to from.

So propably

struct sockaddr_in* from;

is a typo and should have read:

struct sockaddr_in from = {0};

?