Thread: listening loop problem

I'm writing an app that involves a listening loop that listens for and accepts new connections when the user clicks a menu item in the GUI. here is my listening function, which works fine like this:

Code:

int Listen(void)
{
	while(bListen)
		MessageBox(NULL, "test", "test", MB_OK);
			
	return 0;
}

when the "Start Listening" menu item is clicked it assigns TRUE to bListen, and calls this function. the message box is displayed, when I hit "OK" it is displayed again, and repeats until the "Stop Listening" menu item is clicked (which assigns FALSE to bListen).

now, here it is with a little bit of socket code added.

Code:

int Listen(void)
{
	WSADATA wsad;
	SOCKET lsock, csock;
	SOCKADDR_IN sin;
	int err;

	/* Initiate WinSock 2.2, and create and bind a socket for listening. */
	WSAStartup(0x202, &wsad);

	if((lsock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) == SOCKET_ERROR) {
		err = WSAGetLastError();

		WSACleanup();

		return err;
	}

	sin.sin_family = AF_INET;
	sin.sin_addr.s_addr = htonl(INADDR_ANY);
	sin.sin_port = htons(usPort);

	if(bind(lsock, (PSOCKADDR)&sin, sizeof(SOCKADDR_IN)) == SOCKET_ERROR)
		goto SockErr;

	/* Start listening and accepting new connections. */
	listen(lsock, SOMAXCONN);

	while(bListen) {
		if((csock = accept(lsock, (PSOCKADDR)&sin, NULL)) != SOCKET_ERROR) {
                                                /* Here I plan to create a thread for each new connection. */
			MessageBox(NULL, "hello", "hi", MB_OK);
		}
		else {
SockErr:
			err = WSAGetLastError();

			MessageBox(NULL, "error", "err", MB_OK);
			closesocket(lsock);
			WSACleanup();

			return err;
		}
	}

	closesocket(lsock);
	WSACleanup();
			
	return 0;
}

ignore the MessageBox()s, I just added them for debugging purposes. when it looks like this and I hit "Start Listening", the program hangs, which I assume is because the code in the loop is executing too quickly, resulting in some major CPU usage. I tried adding a Sleep(30000) in the loops block (outside of all if else blocks), however it doesn't seem to help.

is this the problem here, the loop is consuming alot of the CPU causing the program to hang? do you think creating a thread for this function would solve the problem? also if I created a thread for it, should I still add a Sleep() statement (as I believe it would just cause the threads execution to pause, not the entire program like it would if I added a Sleep() to it and called it normally like I had tried)?

any help here would be greatly appreciated. thank you in advance.

Well it will stick inside the accept() until something else makes a connection.

And your use of goto is just plain ugly - jumping into the middle of a while loop, which then returns.

what do you mean it will "stick it in" accept()? shouldn't the call to accept() "stick it in" csock? its not like I'm passing it any addresses to csock.

I realize its not recieving connections, but thats not really the point, it can't be doing this if say for example there are no connections.

MessageBox() will return when the OK is clicked, it returns the code of whatever button was clicked (in this case I think its IDOK). this will cause the loop to pause until I click the OK, which is probably fixing the problem of in the loop, as note that I'm using local variables in the loop, I believe Windows will attempt to store local variables in registers by default. so this loop's code is probably executing very quickly, and is literately bogging down execution to the point where anything else (like messages passed to the window) are being ignored.

I'll make it a thread and I'll add a short Sleep(), something like 3000 milliseconds (3 seconds), to the loops block. Making it one seems better anyways, I can probably do away with the bListen global, make the loop infinite, and use the thread functions to control the thread appropriately.

I know the goto is a little ugly, but its small, it works, avoids repetitive code, and there isn't really much to be jumped over here.

Sorry, it's listen that blocks until a connection is made.

So you want a non-blocking listen?

yeah I came to that conclusion about a half hour ago when I wrote the threaded version:

Code:

unsigned __stdcall Listen(void)
{
	WSADATA wsad;
	SOCKET lsock, csock;
	SOCKADDR_IN sin;
	int err;

	/* Initiate WinSock 2.2, and create and bind a socket for listening. */
	WSAStartup(0x202, &wsad);

	if((lsock = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP)) == SOCKET_ERROR) {
		err = WSAGetLastError();

		WSACleanup();

		return err;
	}

	sin.sin_family = AF_INET;
	sin.sin_addr.s_addr = htonl(INADDR_ANY);
	sin.sin_port = htons(usPort);

	if(bind(lsock, (PSOCKADDR)&sin, sizeof(SOCKADDR_IN)) == SOCKET_ERROR) {
		err = WSAGetLastError();

		closesocket(lsock);
		WSACleanup();

		return err;
	}

	/* Start listening and accepting new connections. */
	listen(lsock, SOMAXCONN);

	while((csock = accept(lsock, (PSOCKADDR)&sin, NULL)) == INVALID_SOCKET) {
		MessageBox(NULL, "here goes 10 seconds", "sleep", MB_OK);
		Sleep(10000);
	}

	closesocket(lsock);
	WSACleanup();
			
	return 0;
}

no more jmps into the middle of loops . after reading the msdn description of accept() thoroughly I realized a few things I had wrong, first of all I was testing the return of accept() against SOCKET_ERROR, when I should have been testing it against INVALID_SOCKET. secondly, with this test I noticed that the message box never appeared (I expected accept() to return INVALID_SOCKET when there was no connection to accept), meaning accept() was not returning any value immediately.

Yeah, non-blocking sockets is what I need to make this work. I plan to get all the traffic through 1 port in a multithreaded client/server system, so I think the sockets will probably have to be non-blocking anyways. I'll take a look into this further tomorrow, too late and too lazy now.

Thanks Salem.