A Rewrite of KB192570: An MFC Asynchronous Socket Example Done Right

Home Back To Tips Page

There was a recent newsgroup posting about asynchronous sockets in separate threads. I pointed out many failures in the code, thinking it had been written by the person who posted the question, who had made a huge number of newbie blunders and errors.  A followup message from the original poster (OP) said that the code was based on Knowledge Base article 192570.

So I went to read the article and the code.  The code represents what is possibly one of the worst examples of asynchronous socket usage ever found anywhere.  It gets threading wrong.  It gets the basic code wrong.  It gets the socket handling wrong.  It commits mind-bogglingly stupid blunders and treats them as if they make sense.

It was so bad that when I describe it to thread-savvy colleagues, I describe it for its humor value. We laugh at how unbelievably poor this example is. Gag lines start out as "And would you believe that...?" and everyone just laughs at how pitiful an example this is. On colleague who is working on his Ph.D. in threading methodologies, and who has developed tools to analyze multithreaded systems, commented "The first thing to understand about threading is that everyone gets it wrong all the time.  That's a close enough approximation to the truth to be applicable everywhere."  I'd love to apply his tools, but they only work on Java code. 

The existence of an example this poor is deeply offensive.  Essentially, I rewrote it for "best practice".

You may notice, if you compare my code with the Microsoft original example, that my code is more complex.  However, it has one real advantage over the Microsoft code: it actually works!  It is not clear the Microsoft code ever worked correctly, and it had no provisions for error detection and reporting.

One of the key lessons here is that it is HARD to write robust, reliable, multithreaded code, and you can't toss off a one-afternoon example and expect it is going to be right.  I have spent DAYS making sure this code is correct, and documenting it.  The toy example in KB192570 is just that: it is a toy.  You don't use toy trucks to haul rocks at real construction sites, and you don't use toy examples to build robust and reliable multithreading code.  The first time you put a large rock in the toy truck, it breaks.  The first time you put a real application on the toy code, it breaks.

Article overview

• What's wrong with it?
• Much better practice
• The Rewritten App
• Code details

What's wrong with it?

• Overview
• Questionable practice
• Plain bad practice
• Bad threading practice
• Bad MFC practice
• Bad GUI practice
• Bad network practice
• Bad C++ practice
• Bad Unicode practice
• Bad Visual Studio practice
• Bad documentation practice

Overview

Other than the fact that nearly everything is wrong with it, what can I say?  The code is among the poorest I have seen.  It violates so many principles of good coding (not to mention good MFC programming, good thread coding, proper socket handling, and common sense) that I finally gave up writing down every change I made, and just sat down and rewrote it. 

It is not even a good instructional example.  It has poor comments, illustrates very poor practice in a huge number of ways, and is not a good model for derivative code.

Overall, the original code had 1800 SLOC, the rewrite has 3616.  This includes 851 lines of comments I added. Of the original code, 549 lines were removed or changed.  Not counting the lines generated by the AppWizard and ClassWizard, the original code had about 700 lines that were application-specific, and considering I changed 549, this meant that approximately 80% of the original lines were either discarded or recoded. 

I did another comparison.  The socket and threading code in the original example is, eliminating comment lines, blank lines, and boilerplate VS6 stuff, approximately 650  lines.     My code has approximately 960 lines of socket and threading code.  Not a big difference in terms of quantity, but my code actually works, and works in all project configurations. Not counting lines the 194 lines that consist of '{' or '}', of the 112 lines that remained unchanged, 52 of them were generated by wizards and 31 were simple statements like 'else', 'break' and 'return', which means that only about 30 lines survived into the current version completely untouched.  An additional 44 were isomorphic to renaming, meaning that of the original 456 lines, only  ⁷⁴/₄₅₆ or about 16% actually survived into the rewrite.  So when I say that it was mostly wrong, there are some numbers to prove it.

 It took me a day to rewrite it, and another day to study the code very carefully and make it more robust.  Little things like detailed error checking and reporting, handling multiple Send requests with a queue, creating a stress-test situation for those multiple-send operations, doing a slicker GUI interface, and adding some additional TRACE statements occupied most of that second day.  The core technology, including the FSM, handling and reporting network errors, making it Unicode-aware, and making it 64-bit compatible, took about ten hours; it would have taken less time if VS2005 had not been so tedious to deal with, and would have gone even faster if VS2005 had a mean uptime greater than about an hour before it crashed without warning, thus losing some of my changes and requiring I redo them.  It took me four more days to write this Web page.  One entire day was spent creating hyperlinks.

Frankly, it looks like a project written by a summer intern between his or her freshman and sophomore years, who had spent a week studying MFC and had once heard threads described over the telephone.  And had once, perhaps, read a book on socket programming, but had forgotten most of it.  Where was the adult supervision at Microsoft that approved a project this bad for distribution in the MSDN?

Essentially, this code in the MSDN example is a textbook case of some of the worst possible programming practices that can be used to write any code, anywhere, at any time.  It is incidentally bad networking practice, bad MFC practice, and bad threading practice.

Questionable practice

There is no discussion of why threading is required to handle multiple connections.  Generally, there is no need to use separate threads to handle the multiple asynchronous sockets.  Even if the message that comes in from the socket requires considerable computing, the more effective method is to have all the sockets running in the main GUI thread, and have the extensive computing handled by threads from a thread pool.  This would be a better approach to the design.

However, given the premise that there is a desire to run each socket in a separate thread, the KB example is without a doubt one of the worst examples of how to do multithreaded sockets that could be created.  It isn't even a good framework to start with, given that I did a total rewrite of the algorithms.

Plain bad practice

A number of project-only constants were used in stdafx.h. This is exceptionally poor style, since  stdafx.h is  reserved for standard header files. Project-related constants should be in separate header files.

In addition, this represents a very bad practice known as "lumping", tossing random constants, external declarations, etc. into a single header file.  What was in it?  Well, some message numbers, a buffer size, and a port number.  These represent completely unrelated concepts and belong in completely independent header files.  It is a common newbie error to think that more files are bad.

The values lumped in here were

#define WM_THREADSTART WM_USER+200
#define WM_THREADCLOSE WM_USER+201
#define WM_NEWSTRING WM_USER+202
#define WM_TERM_THREAD WM_USER+203

#define MAX_BUFF 4096
#define PORT_NUM 9898

I moved the messge declarations to a separate file called messages.h.  I moved the port number declaration to a separate file called port.h.  And I got rid of the MAX_BUFF value because it had no reason to ever exist in the first place.  Furthermore, the messages used by the client and server were similar, but each project declared its own local copies of messages in its own stdafx.h file.  This was just silly.  Instead, I put the messages.h file in the shared include directory.  I put the shared directory in the project settings of both

The idea of lumping is a lazy programmer's attempt to not do the job right.  There is no excuse for lumping unrelated concepts into a single header file.  I've been known to create a single-line header file (well, two lines, counting the #pragma once) with a single external function or const or #define in it.  Every once in a while, I'll create a global variable, but only after careful thought about its need to be global.  This rarely happens.

In fact, the whole idea of lumping has been proven to be a highly-ineffective development methodology.  With numbers to back up this observation.  See Ellen Borison's Ph.D. dissertation. Program Changes and the Cost of Selective Recompilation, Carnegie Mellon University, Department of Computer Science, 1989.. Her conclusion: the more files you have, the faster your development cycle will proceed. 

There were still lots of things wrong just with these declarations, which I will also address later.

What else is wrong with the project?

Just to show how bad this code is, these constants are DUPLICATED in the stdafx.h file of the client project! So what I did was move the header files of the shared constants to a separate include directory and put that include directory in the project search path of each of the two projects.

The messages are named with a WM_ prefix, which should be reserved for Microsoft-defined messages. So I changed them to UWM_ messages (User Window Message). The values were based on WM_USER, which should be reserved for control-specific messages. I changed them to WM_APP-based messages. Frankly, I consider this poor practice and I would have used Registered Window Messages. But I'll let it go. But I certainly DOCUMENTED the messages!

CAsyncSocket methods which could fail were not tested for success.  Perhaps one of the most egregious failures was the Accept statement, which was not tested for success, but if it failed, the only possible outcome was catastrophic failure that would nearly impossible to analyze.  Errors were either not detected, or detected but not reported.  This is unacceptable practice.

The client code had all the problems of the server code, and in addition it used slightly different variants of the code that sent slightly different messages. The code was changed so that the same connects.h/connects.cpp files are used both by the client and the server, and they use the same messages and the same protocol.

Bad threading practice

The mechanism for passing information to the thread involves a variable. It is also clear that the person who wrote this was clueless about any kind of interthread synchronization. Rather than write a correct program, the program was defined by design to be erroneous, reporting only the most recently received string. An example program should be designed to be an exemplar of correct implementation of a correct design, not a poor implementation of a bad design. This program violates that design principle.

The bad practices this represents is

• Using a single variable to pass information between a secondary thread and the main thread
• Using locks when locks are not needed
• Implementing some kind of weird PeekMessage loop to discard redundant messages (the concept that these messages are redundant already demonstrates the author ws completely clueless!)
• Creating an information-losing situation guaranteed to cause data lossage.

Thread shutdown was handled badly and this has been fixed.  The way it worked was that a notification was sent to all the server threads to shut down, and then the program waited 5 seconds in the delusional belief that this would guarantee some form of correctness.  This is patently absurd.  There is only one correct way to handle this and that is that as each thread shuts down it sends a notification, and when enough notifications have been received, the main thread initiates its shutdown.

The bad practice this represents is a proclivity among beginners to try to write sequential code in an asynchronous environment.  For example, if three things are to be executed, as in A(); B(); C();, and the desire is to make these into threads. the typical blunder is to write it as

CWinThread * Athread = AfxBeginThread(A,, NULL);
::WaitForSingleObject(Athread->m_hThread, INFINITE);
CWinThread * Bthread = AfxBeginThead(B, NULL);
::WaitForSingleOjbect(Bthread->m_hThread, INFINITE);
CWinThread * Cthread = AfxBeginThread(C, NULL);
::WaitForSingleObject(Cthread->m_hThread, INFINITE);

Note that there are a huge number of errors in this paradigm.  For example, CWinThread objects will be automatically deleted (and the handle closed) when the thread terminates.  So the ::WaitForSingleObject exits not because the thread handle became signaled, but because it became illegal.  Doing this sort of protocol correctly requires a lot more code than is shown here, and yet there are people who deliver what they think are "working" products written this way (I've had to fix them).

It throws away any advantage of threads; by blocking the main GUI thread, the GUI is dead.  It can't respond to user input, it can't log messages from the processes, it can't deal with WM_TIMER messages, and so on.  So there is no reason to have used threads at all! 

But the real problem is that it forces a synchrony on what is a fundamentally asynchronous mechanism.  The correct approach is to start the thread doing A, and when it completes it sends a notification to the main thread that indicates that A is done, and the thread then initiates B.  The thread A doesn't have to know that B will follow.  Suppose that a single parameter value has to be passed to each of them, just to show how this can be done.

AfxBeginThread(A, p);

/* static */ UINT whateverclass::A(LPVOID p)
   {
    CSomething * parms = (CSomething *)p;
    ...do stuff...
    parms->target->PostMessage(UWM_A_DONE, (WPARAM)p);
    return 0;
   } // whateverclass::A

LRESULT CTargetClass::OnADone(WPARAM wParam, LPARAM)
   {
    AfxBeginThread(B, (LPVOID)wParam);
    return 0;
   }

and so on.  We will look at the shutdown for multiple concurrent threads later.

In creating the threads, the constructor for the server thread had an InterlockedIncrement call on a variable. The comment erroneously states that it records the number of threads running; it actually records the number of CServerThread objects created. It is not clear what purpose this variable serves, since a properly-done multithreading system doesn't require the use of an interlocked variable to maintain the count of threads.

The InterlockedDecrement doesn't actually record the fact that the thread has stopped; in addition, the destructor is using a SetEvent which is erroneous because the threads should not be managing this task.  And the main GUI thread should not be blocked waiting for such a notification.

In ClientT.cpp, the ExitInstance handler engages in cross-thread SendMessage. This is seriously erroneous because the main thread might actually be blocked!

For reasons that appear to make no sense, the CWinThread pointer returned by the AfxBeginThread was stored in the socket that was stored in the thread.  Code of the form

pThread->m_strServerName = str; // server machine name
pThread->m_socket.m_pThread = pThread; // the thread that m_socket lives

can best be described as sloppy, and it has the potential to be erroneous.  Why is it that the creator of the thread knows that there is a member called m_Socket and that it has a member called pThread?  For that matter, since it makes no sense to have the thread available, inside the socket, and it is not used inside the socket, there seems to be no rationale for doing this at all.  Why would there need to be a reference to the CWinThread object inside the socket?  This is not good OO programming, and it makes no sense.  The thread handle can be obtained, if needed, by ::GetCurrentThread(), and the thread ID, if needed by ::GetCurrentThreadId().  So there is no need for any reference to the CWinThread object.

The sending logic is completely broken.  If a long message is sent, and part of the sending involves a WSAEWOULDBLOCK return, then the sending thread must wait for an OnSend callback to send the next part of the buffer.  But if an AsyncSendBuff call is issued from the dialog thread during this time, it will completely corrupt the data structures of the program.  This is unbelievably sloppy programming.  There is no synchronization between the two threads to ensure that this cannot happen, and there is no queuing mechanism to make sending work correctly!  There is a comment in the code

// We don't queue up data here.
// If you are going to queue up data packet, it would be better to limit the size
// of the queue and remember to clear up the queue whenever the current packet has been sent.

The first is an observation that the code is erroneous.  The second is an observation that the size of the queue should be limited, although I cannot exactly decide why such a limit needs to exist, and in any case there would be better ways to handle it than are suggested by this comment.  And finally, what is not screamingly obvious about the need to remove an element from the queue, and more likely, before it has been sent.  I suspect the reason a queue was not implemented here is that the author was completely clueless as to how to do it.  And, as you will read in my implementation, it isn't an easy thing to deal with.  But at the very least there should have been a test to see if the previous packet was still in transmission, and to return FALSE if it was; not just proceed on and do bad things.  For example, the test

if (m_nSendDataLen != 0 || nBufLen > MAX_BUFF)
    {
     TCHAR szError[256];
     wsprintf(szError, _TEXT("CConnectSoc::AsyncSendBuff() can't accept more data\n"));
     AfxMessageBox (szError);
     return;
    }

contains so many design errors it is mind-boggling.  This function is called in the context of the main GUI thread.  It is accessing two variables, m_nSendDataLen and nBufLen, which are being modified concurrently in the socket thread, with no form of synchronization.  Then in allocates a fixed-size buffer, uses the dangerous and obsolete wsprintf to format the message, issues a blocking AfxMessageBox call, and simply returns without any indication that there has been a failure! 

Note that it confuses two independent cases: a case where there is an active data transmission in progress, and the case where an incorrect buffer size was given.  It then issues a misleading message.  Note that some end user of the product that contains this library cannot possibly understand what this would mean, or what could be done about it, or the implications.  This might spell total catastrophe, but the programmer who calls this function could never find out that it had failed, or why!

We do not need bad examples of poorly defined interfaces for people to model.

Correct code would set a lock, check the variables, not do any formatting at all, not issue an AfxMessageBox call, and simply return FALSE. The policy of what do do in this case belongs to the caller, and implementing an clearly absurd policy of popping up a Message Box in English is not just poor design, but abysmal design.

Perhaps one of the single worst commercial products I used, a database library, had the misfeature that if it felt you had made an error, it wou/d call ::MessageBox with the desktop as the parent.  This meant that the program would seem to hang, but the message box was underneath the app, and since it had the desktop as a parent, it didn't disable the GUI as a modal dialog box would.  It issued a meaningless error message (to the end user), and returned no indication of error to the programmer.  If you managed to find the dialog, and clicked OK, it would then exit your program.  It was one of the worst exemplars of decent error reporting and recovery I ever saw,  It came with source code, which was so badly written that I decided we had to abandon that product, because nothing short of a complete rewrite could salvage it. 

Here's a real classic:

    ASSERT(m_nBytesRecv < MAX_BUFF && m_nRecvDataLen <= MAX_BUFF);
    nRead = Receive(
                    m_recvBuff + m_nBytesRecv,
                    m_nRecvDataLen-m_nBytesRecv);

Note that it will issue an ASSERT if the size of the packet is larger than the fixed buffer size, but only in the debug version!  It will happily read too much data into the buffer in release mode.  It is this kind of unbelievably slovenly coding that results in buffer overruns in released products--ASSERT is only for debugging, not correctness.  Even in the debug version, after the ASSERT is issued, it still goes ahead and reads more data into the buffer than there is buffer to read it into!  Code like this is so grotesquely irresponsible that it must be killed off.  How anyone could have been allowed to let an example this bad into the MSDN shows that there is truly no adult supervision of these examples.

Bad MFC practice

Controls are manipulated directly instead of using control variables. This has been fixed.

The bad practice here is using GetDlgItem, SetDlgItemText, or SetDlgItemInt to manipulate controls.  Instead, create control variables and manipulate the controls using those.  I feel strongly that anyone who writes more than one or two GetDlgItem calls per year is not using MFC correctly (there are rare exceptions dealing with arrays of controls, and they don't happen very often).

There are places in which a clumsy call to PostMessage is done

::PostMessage(AfxGetApp()->m_pMainWnd->m_hWnd, UWM_THREADSTART, ...);

when a simple PostMessage method would have been better.

AfxGetApp()->m_pMainWnd->PostMessage(UWM_THREADSTART, ...);

More seriously, because this code presumes the main application window is going to get the messages, it is not reusable in a situation where views might be involved, or any other kind of window.

The code is written in MFC, yet the neophyte who wrote this uses fixed-size buffers and wsprintf to format data, instead of using CString::Format. This code is unsalvageable.

The error here is using obsolete and unsafe API calls that can cause buffer overruns, when there is a simpler and reliable mechanism already in MFC.

There are places where OutputDebugString (unconditional tracing call) is called instead of TRACE. These have been fixed.

As is typical for all projects I work on, every trace of the project.h file (AsyncClient.h, AsyncServer.h) were deleted from every file except the corresponding AsyncClient.cpp and AsyncServer.cpp. Microsoft loves to toss these #includes into every generated class file, and they make absolutely no sense to be in any file except the CWinApp-derived class.  So while this represents bad MFC practice, at least this isn't the fault of the original programmer.

There were several places in which the user-defined message handlers were defined to take parameters of UINT, LONG instead of the correct WPARAM, LPARAM. These were fixed. This code would not compile and run properly in a 64-bit environment.

There were several places in which the thread pointer was put into the thread object, even thought it was not used.  In fact, there were a large number of gratuitous variables that were initialized but never used, but which should never have existed at all!  These were all removed.  There is no reason a socket would ever need access to its CWinThread object; if it does, the design is erroneous.

Most methods that had been declared as public were gratuitously public and did not need to be public.  Admittedly, Microsoft got this wrong and largely still has it wrong; for example in VS.NET, all handler methods are declared as public, even though this makes absolutely no sense.  It is necessary to examine every public object to make sure it needs to be, and is not the result of sloppy programming, whether on the part of the programmer or on the part of Microsoft.

Bad GUI practice

The use of OK and Cancel buttons is poor style for dialog-based apps because they could be closed by a stray <enter> or <esc> key.

To fix this, I modified the OK and Cancel handlers and  removed their bodies.  Then I removed the buttons entirely.  I added code to the OnClose handler directly calls CDialog::OnOK() when it is time to shut down. A new button, Close, has been added and an OnBnClickedClose handler initiates shutdown. I describe this in my essay on dialog-based apps.

This code issues AfxMessageBox calls from secondary threads. This is erroneous and has been fixed.  This could also have been listed as bad threading practice.  There is rarely a need to stop a thread, and if there is an error, it can generate notifications in the main GUI thread.

Controls are created as enabled or disabled.  Control state should be computed at run time under all conditions.  Control state should be computed in exactly one place, and based on a dynamic computation (not a bunch of random pieces of code that set Boolean values).  See my essay on Dialog Box Control Management.  I have implemented that model in these apps.

The client code did insane things like SendMessge(WM_CLOSE) to its parent, instead of doing PostMessage(WM_CLOSE). It would be erroneous to do SendMessage.

The GUI will block during shutdown waiting for the threads to shut down, but give the illusion that it is live.  As a general practice, I try to avoid ever doing a ::WaitForSingleObject in the main GUI thread.  Again, the notion that what you want to do is

void CMyMainWindow::OnClose()
   {
    initiate thread shutdown;
    wait for all threads to shut down;
    CMyParent::OnClose();
   }

is another misguided attempt to impose sequentiality on an asynchronous process.  The correct form is

void CMyMainWindow::OnClose()
   {
    if(any threads running)
       { /* asynchronous shutdown */
        set shutdown-in-progress flag;
        initiate thread shutdown;
        return;
       } /* asynchronous shutdown */
     CMyParent::OnClose();
    }

As the thread shutdown sequence progresses, eventually someone notices that there are no more running threads and the shutdown-in-progress flag is set.  At that point, the code looks like

    if(number_of_running_threads == 0 // by some determination
        && shutdown_in_progress)  
       PostMessage(WM_CLOSE);

This removes all need to wait for a shutdown.  In a dialog-based app, some or all controls can be disabled; in an MDI or SDI app, the ON_UPDATE_COMMAND_UI handlers would use the shutdown-in-progress flag to decide what menu items need to be enabled.

(A classic argument against this goes something like "<whine> that takes a lot of work!" and the answer is "Yes it does.  Get over it.".  You can do the job right, or you can do the job wrong.  Products that do the job wrong do not generate happy users.)

Bad network practice

The port number is 9898. This is in the "reserved port number" section of the assignable port numbers, and therefore could potentially conflict with a Registered Port Number. For example, checking the Registered Port Number list at www.iana.org/assignments/port-numbers, we see that this actually is a conflict with a registered port number. So I changed it to 0xC001, an equally random number but one that at least is safe. It is in the private port region.

In a well-designed program, the port address would be settable from a GUI component and stored in the Registry.  I have therefore added this feature, insofar as the GUI component.  I did not save it to the Registry, but I recommend my Registry Classes library for this purpose.

The sending logic is confused. For example, it does a

Send( (LPCTSTR)m_SendBuffer + m_nBytesSent, m_nSendDataLen - m_nBytesSent);

but there is no rationale of why m_SendBuffer, which is declared as a TCHAR, needs to be cast to an LPCTSTR, particularly because the parameter specification for Send is specified as LPVOID; the result is also erroneous in that the arithmetic will not work in a Unicode build. For these purposes, the buffer should be declared as a BYTE buffer. No cast is required.

Like far too many programmers, the person who wrote this somehow assumes that a Receive will receive enough data to be meaningful. In addition, the original programmer chose to use fixed-size buffers, which is nonsensical. I rewrote the receive logic to use a Finite State Machine model which is easier to understand, code, and maintain.

When network callbacks occur (the virtual methods such as OnSend, OnReceive, OnConnect, the error code is ignored.  This is clearly erroneous.  This led to cases where, for example, a failure to connect resulted in the client thinking it actually had a connection.

Bad C++ practice

Here's an excerpt from one of the header files.  It was apparently intended by the programmer that the connects.h/connects.cpp files be shared, but this was not achieved in practice.  But look at this example:

class CConnectSoc : public CAsyncSocket
{
public:
	CConnectSoc();
	virtual ~CConnectSoc();
public:
#ifdef _ASYNCSERVER
	// Pointers to members in CServerDlg

	CString* m_pLastString;
	CCriticalSection* m_pCriticalSection;

	CWinThread* m_pThread;
#endif

        ...
};

What was this person thinking?  A piece of code like this screams "subclassing".  Conditional compilation like this is an exceptionally poor style to use to customize a class for two purposes.  In C++, you would create a superclass, perhaps an abstract superclass (not easy in MFC for reasons discussed below), and then specialize it to the given need by deriving subclasses!  Code like this is inexcusable.

In addition, it is not at all clear why these pointers are used. 

Of course, the fundamental design error that a single CString is used to pass data back from the network thread to the main thread represents an absolutely insane concept, particularly because there is no effort to truly synchronize access so once data is put into the variable, the variable is inaccessible until the data has been removed. This creates that data-losing situation I described earlier.  If such synchronization were provided, it would create a massive performance bottleneck.  So this variable is erroneous in the design. 

The CCriticalSection shows that the programmer actually believed the MFC synchronization primitives are usable, which they are not, and putting a shared pointer here might have been useful, had the need for synchronization been done right.  But I would have used a CRITICAL_SECTION *, except that I believe that the whole notion of interthread synchronization done this way is erroneous.  Proper synchronization would have used a queue and semaphores, and the synchronization primitives would ideally have been hidden in the abstractions of the queue.  For more information on how semaphores would be used, see my essay on semaphores.

Finally, the CWinThread object is totally useless.  Its role is undefined.  It is not actually used in the code, and there is no reason to have it at all, for any purpose.  There is nothing in a CWinThread that is conceivably useful for a socket thread.

Bad Unicode practice

I've already pointed out where the failure of address arithmetic occurs in computing offsets into the Send buffer.  But the code is riddled with Unicode-related errors.

The original apps contain so many coding errors that if they were compiled as Unicode they would completely and utterly fail. This sort of example is no longer tolerable under modern practice.

The code would not work between machines with different endianness.  The code would not work if one app were Unicode and the other were ANSI. 

For that matter, if data were transmitted in Unicode, it would be necessary to convert each Unicode character into Network Standard Byte Order.  Alternatively, you could start the Unicode stream with a Unicode Byte Order Mark (BOM), which would indicate if the Unicode characters were stored in Little Endian or Big Endian order.  This would require an agreement between the sender and receiver to put the BOM as the first Unicode character.

The code reverses the bytes and sends them back.  This works only if characters equal bytes.  Or, in Unicode, if characters are code points.  But Unicode surrogates mean that this will not work correctly, because swapping the code points will produce erroneous Unicode streams.  This misfeature was removed.

Bad Visual Studio Practice

The server and client are essentially independent projects, yet they were done with the one as a subdirectory of the other.  If projects are independent peer projects they should be independent sibling directories under an umbrella parent project.

Bad Documentation Practice

Here's a list of messages.  What do they do, and what are their parameters?

#define WM_THREADSTART WM_USER+200
#define WM_THREADCLOSE WM_USER+201
#define WM_NEWSTRING WM_USER+202
#define WM_TERM_THREAD WM_USER+203

You can't go to the definitions of the hander functions; these are not documented either.  You have to read the code to figure out what is going on here!  It is a common practice with C programmers to think "I've written the code; it is obvious how to use it just by reading it!"  This is the excuse to not do decent documentation of the code.  I've been using the same documentation style for nearly 30 years.  It is reflexive to use it.

Much Better Practice

There are several interpretations as to what constitutes "best" practice.  Some are seriously debatable.  The one thing we can be absolutely certain of, beyond any shadow of a doubt, is that the original Microsoft example does not even come close to mediocre practice, and in some respects represents worst practice.  Everything I have done here uniformly represents massive improvement.  As to whether or not it is "best", well, that's arguable.

• Better Coding practice
• Better Documentation practice
• Better Network practice
• Better MFC practice
• Better GUI practice
• Better Unicode practice
• Better Localization practice
• Better C/C++ practice
• Better Tracing practice

Coding practice

Constants which were project wide but duplicated were placed in a common set of header files and placed in a common include directory called include. 

Code which was the same in both projects was placed in a common directory called, strangely enough, common.

Expressions in #define constructs were parenthesized.

Names were changed to not conflict with reserved Microsoft constants.

I changed the parameters to messages so that if a thread ID is passed for any reason, it is always passed in LPARAM.

Document messages as if they are methods!

Messages are part of the formal interface.  There is nothing quite so amateurish as a sequence of declarations of user-defined messages like the one I found.  I changed it to treat messages as interfaces, and as such, I documented every message (this is the minimum acceptable standard I will accept).

Believe it or not, I once had a client complain that these comments "made the file too hard to read".  I pointed out that the only important part of the file was the comments, and the #defines were mere implementation details, hardly worthy of notice.

/****************************************************************************
*                               UWM_THREADSTART                              
* Inputs:                                                                    
*       WPARAM: unused                              
*       LPARAM: (LPARAM)(DWORD) Thread id of thread 
* Result: LRESULT                                                            
*       Logically void, 0, always                                            
* Effect:                                                                    
*       Notifies the main GUI thread that the child thread has started       
****************************************************************************/
#define UWM_THREADSTART  (WM_APP+200)                                        
 
/****************************************************************************
*                               UWM_THREADCLOSE                              
* Inputs:                                                                    
*       WPARAM: unused                                                       
*       LPARAM: (LPARAM)(DWORD) Thread id of thread 
* Result: LRESULT                                                            
*       Logically void, 0, always                                            
* Effect:                                                                    
*       Notifies the main thread that a secondary thread has terminated      
****************************************************************************/

#define UWM_THREADCLOSE  (WM_APP+201)                                        
 
/****************************************************************************
*                                UWM_NETWORK_DATA                               
* Inputs:                                                                    
*       WPARAM: (WPARAM)(CByteArray *) Pointer to the data that was received 
*       LPARAM: (LPARAM)(DWORD) Thread id of thread 
* Result: LRESULT                                                            
*       Logically void, 0, always                                            
* Effect:                                                                    
*       Passes data to the main thread to be processed and/or displayed
* Notes:                                                                     
*       It is the responsibility of the recipient to delete the CString      
****************************************************************************/

#define UWM_NETWORK_DATA    (WM_APP+202)                                        
 
/****************************************************************************
*                               UWM_TERM_THREAD                              
* Inputs:                                                                    
*       WPARAM: unused                                                       
*       LPARAM: unused                                                       
* Result: LRESULT                                                            
*       Logically void, 0, always                                            
* Effect:                                                                    
*       Sent via PostThreadMessage to the child thread to shut it down       
****************************************************************************/

#define UWM_TERM_THREAD  (WM_APP+203)                                        

/****************************************************************************
*                            UWM_CONNECTIONCLOSE
* Inputs:
*       WPARAM: unused
*       LPARAM: (LPARAM)(DWORD) Thread id of thread 
* Result: LRESULT
*       Logically void, 0, always
* Effect: 
*       Notifies the main thread that the connection has closed
****************************************************************************/

#define UWM_CONNECTIONCLOSE (WM_APP + 204)

/****************************************************************************
*                             UWM_CONNECTIONMADE
* Inputs:
*       WPARAM: unused
*       LPARAM: (LPARAM)(DWORD) Thread id of thread 
* Result: LRESULT
*       Logically void, 0, always
* Effect: 
*       Notifies the main thread that a connection has succeeded
****************************************************************************/

#define UWM_CONNECTIONMADE (WM_APP + 205)

/****************************************************************************
*                                  UWM_INFO
* Inputs:
*       WPARAM: (WPARAM)(CString *) Connection information message
*       LPARAM: (LPARAM)(DWORD) Thread id of thread 
* Result: LRESULT
*       Logically void, 0, always
* Effect: 
*       Used to post trace info about message receip, connections, etc.
* Notes:
*       This only applies when the socket is
*       set to do this (see CConnectSoc::SetInfoRequest)
*       The recipient is responsible for the deletion of the string
****************************************************************************/

#define UWM_INFO (WM_APP + 206)

/****************************************************************************
*                              UWM_NETWORK_ERROR
* Inputs:
*       WPARAM: (WPARAM)(DWORD) error code
*       LPARAM: (LPARAM)(DWORD) Thread id of thread 
* Result: LRESULT
*       Logically void, 0, always
* Effect: 
*       Notifies the main thread that a network error has occurred
****************************************************************************/

#define UWM_NETWORK_ERROR (WM_APP+207)

/****************************************************************************
*                              UWM_SEND_COMPLETE
* Inputs:
*       WPARAM: unused
*       LPARAM: (LPARAM)(DWORD) Thread id of thread that completed sending
* Result: LRESULT
*       Logically void, 0, always
* Effect: 
*       Notifies the owner that the last Send has completed
****************************************************************************/

#define UWM_SEND_COMPLETE (WM_APP+208)

/****************************************************************************
*                                UWM_SEND_DATA
* Inputs:
*       WPARAM: (WPARAM)(CByteArray*) Reference to a CByteArray to be sent
*       LPARAM: unused
* Result: LRESULT
*       Logically void, 0, always
* Effect: 
*       Enqueues a send request
* Notes:
*       This is sent to the thread via PostThreadMessage
*       The sender must allocate the CByteArray on the heap.  The network
*       layers will delete this object when it has been sent
****************************************************************************/

#define UWM_SEND_DATA (WM_APP + 209)

/****************************************************************************
*                            UWM_START_NEXT_PACKET
* Inputs:
*       WPARAM: unused
*       LPARAM: unused
* Result: void
*       
* Effect: 
*       This is sent via PostThreadMessage from the network thread to itself
*       to initiate the asynchronous dequeue of the next pending packet
****************************************************************************/

#define UWM_START_NEXT_PACKET (WM_APP + 210)

Every function that is in my code, even the functions generated by the ClassWizards, gets a header like one of the above.  There is no excuse to not be at least this thorough in writing code.  Messages are just functions with a different wrapper, and they deserve to be documented as first-class entities.  The nice thing is that my editor can drop one of these headers in with about three keystrokes and the name of the function.  Since it can also generate an empty function body, this means it takes me two keystrokes and the name of the function to add the function and its documentation in a single operation. (It takes one extra keystroke to say "Drop in a header, the body is already here").  And people wonder why I don't use the VS editor!   It is hard to respect a program editor that can't even get indentation right!

Network practice

The port number was moved from the reserved area (where it actually conflicted with an official IANA-registered port number) to the temporary/private port number area.

The port number can be changed at runtime.  This is essential to allow in an product code, unless you have an IANA-registered port number, and even if you do, it is still good practice to allow it to be set, because a target site might have a business-critical app that is erroneously coded to conflict with your registered port number, and you should accommodate this situation.

The Receive call is implemented as a finite-state machine which can read partial values for the length and will read arbitrary length strings.

All callbacks that take an error code parameter actually check the error code.  At the moment, they all terminate the connection, but unlike the previous example, they actually notice that an error occurred, and they report the error.  The previous example simply ignored the errors, which could lead to cascading error conditions, and failure to report serious problems (try connecting to a nonexistent server in the Microsoft example.  Study carefully the highly informative error message that tells you the server was not found.  Oh, you didn't see it?  Hmmm... maybe because it isn't there?)

MFC practice

I believe that objects such as collections should be used to manage sequences, rather than raw allocation of bytes, whenever possible.  I have changed the code to use CByteArray as the means of communication of network buffers.

To retain neutrality, I assume that the data might be binary and therefore contain embedded NUL characters, making representation of a string problematic.  So all data is passed into the messaging level as a CByteArray and all received data is sent back from the messaging layer as CByteArray.

Since these apps are passing string data, at the application level (not at the network level) I convert the text strings to UTF-8 representation for transmission. This allows interchangeability between Unicode and ANSI applications.  Note, however, that the representation displayed in an ANSI app may contain ? characters to indicate UTF-8 encoding that cannot be represented as ANSI 8-bit characters.

Member variables and methods not required to be public should (and are) marked as protected. 

Member variables are assumed to be non-public.  What is declared public are Set and Get functions to update the member variables.  In many cases, the Get functions are not public, or do not exist at all, because the data is write-once, and is read only from within the class.

The CClientThread and CServerThread have so much code in common that I declared them as subclasses of a more general CSocketThread class derived from CWinThread.  Thus the implementation of CClientThread and CServerThread is fairly trivial, with all work being done in the CSocketThread.

I finally got frustrated at the stupidity of VS that puts the CAbout class in the application class and ripped it out to a single pair of files, About.cpp and About.h.  This is because the two About boxes are essentially identical.  It has never made sense, in the entire history of MFC that the CAbout class was not placed in a file of its own.

GUI practice

I centralize all control enabling in a single repository of knowledge, the updateControls method.  Any change that could result in a change in the enabling/disabling, visible/invisible, or any other state of the controls is computed here, and here alone, based on information it derives on demand from the environment.  Much of this information is related to control contents; some is related to state such as having a network connection active.

To make this work, controls that require non-empty values in other edit controls in order to be enabled are disabled.  This eliminated a lot of gratuitous testing that went on in a lot of the functions, because they simply cannot be called if their preconditions are not met.  This saves popping up annoying MessageBoxes during execution.

Controls that can have no effect after a connection is established should be disabled, such as the IP address, port #, etc.

I renamed the messages to be UWM_ messages, but I left them as WM_APP-based messages (a significant improvement from their former WM_USER base).  In my own code, I would never consider using WM_APP-based messages.  I would use Registered Window Messages, as I describe in my accompanying essay.

While it is often the case that an example is stripped down to the basics, the "basics" do not make good examples.  Therefore, I've added features like a ListBox that logs the various messages, state changes, and notifications, and has the additional property that it does not scroll if the last element was not visible when something is appended.  There are few systems more inconvenient than those that do not scroll to show the latest output, and few systems more offensive than those that insist on scrolling to the end when you've scrolled back to look at something.  The ListBoxes used in this project pass both tests of convenience and non-offensiveness. 

My own "best practice" for logging is to use my Logging ListBox Control.

Unicode practice

In order to demonstrate bidirectional transfer between the client and the server,  the server application adds some punctuation and sends the string back.  This means that it will work correctly in the presence of Unicode surrogates.  The original application simply reversed character order, that is, code point order.  It would not work in the presence of Unicode surrogates.

By default, the standard dialog font does not have a full Unicode character set; it has only the characters U0000..U00FF.  So in the OnInitDialog handlers of the applications, I create a Unicode font and replace the fonts of the controls that will have Unicode characters in them. Since this only applies when Unicode characters are possible, I put this under the _UNICODE conditional compilation option.

#ifdef _UNICODE
    if(CreateUnicodeFont(this, font))
       { /* got a font */
        c_LastString.SetFont(&font);
        c_Record.SetFont(&font);
       } /* got a font */
#endif

BOOL CreateUnicodeFont(CWnd * w, CFont & font)
    {
     CFont * f = w->GetFont();
     ASSERT(f != NULL);
     LOGFONT lf;
     f->GetLogFont(&lf);
     StringCchCopy(lf.lfFaceName, sizeof(lf.lfFaceName)/sizeof(TCHAR), _T("Arial Unicode MS"));
     return font.CreateFontIndirect(&lf);
    } // CreateUnicodeFont

Localization practice

All strings that are normally user-visible are in the STRINGTABLE so they can be localized.  Debugging messages are normally directed to the programmer during development and therefore they usually remain in the programmer's native language, which in my case means English. However, I have been known to remove all English-language strings, including debugging strings, and put everything in the STRINGTABLE.

C/C++ practice

I created two specialized subclasses of an abstract superclass, CConnectSoc.  One of them holds information for the server sockets, CServerSocket, and one holds information for the client socket, CClientSocket.  The method which will be called to process the packet after receipt is

 virtual void ProcessReceive(CByteArray & ) PURE; // define in subclasses

Therefore, there can never be an instance of a CConnectSoc object.  Since the expectation is that this class must always be subclassed to specify its actual behavior, this is a Good Thing.

Other classes are not quite so fortunate.  For example, there is a substantial amount of common behavior between the CClientThread and the CServerThread classes.  In fact, the only real difference is that one has a CClientSocket for its connection, and the other has a CServerSocket.  I say "nominally abstract superclass" because it is not possible in classes derived from CObject, such as CWinThread, in a manner that  introduces true abstract layers.  If I were to write

 virtual CConnectSoc & GetSocket() PURE; 

which, from the C++ view, makes perfect sense, it won't compile.  The construction

IMPLEMENT_DYNCREATE(CSocketThread, CWinThread)

will produce the error message

ð IMPLEMENT_DYNCREATE(CSocketThread, CWinThread)

I:\mvp_tips\MFCAsync\AsyncClientServer\Common\SocketThread.cpp(20) : error C2259: 'CSocketThread' : cannot instantiate abstract 

                                                            class due to following members:
i:\mvp_tips\mfcasync\asyncclientserver\include\socketthread.h(4) : see declaration of 'CSocketThread'

so instead, I have to write

virtual CConnectSoc * GetSocket() {ASSERT(FALSE); return NULL; } // implement in subclasses

This was a little unfortunate in that I have to return a CConnectSoc * instead of the more reasonable CConnectSoc &, but there is no way to return an "undefined" reference.  Furthermore, I cannot declare something like CConnectSoc dummy; because the CConnectSoc is an abstract superclass and cannot be instantiated. And in any case, I want this to be reported as an error. The comment is there so that if someone gets this error, they will see that they have called a nominal abstract superclass and therefore they cannot proceed until they finish writing that part of the code.

The CClientThread class contains the code

 virtual CConnectSoc * GetSocket() { return &m_socket; }
 CClientSocket m_socket;

and the CServerThread class contains the code

 virtual CConnectSoc * GetSocket() { return &m_socket; }
 CServerSocket m_socket;

I created these projects using error level W4 and "Treat warnings as errors."  (It was interesting that the original project actually does compile under W4 in VS6, at least in the ANSI build.  Since it had no Unicode build and indeed could not produce a valid Unicode version, I did not test it in a Unicode configuration. I was actually surprised that it compiled cleanly at W4).

I did have to make one change to support W4, however.  In some cases, to facilitate debugging, I would write

DWORD err = ::GetLastError();
ASSERT(FALSE);

this made it easier to see what error had occurred.  Due to defective design of the ASSERT mechanism, it resets the last error code, so when the programmer gets control in the debugger, the critical error information has been lost.

But this would generate a warning, 4189, about an initialized but unused variable.  In the modules where I cared, I simply added

#pragma warning(disable,4189)   // allow DWORD err = ::GetLastError() to compile without warning even if err unused

Tracing

When a TRACE statement is issued, it is very good practice to include the name of the function as well.  There are few things less useful than messages that come out and say "Invalid value".  It doesn't matter whether they are done as a MessageBox (see my essay on FormatMessage) or as a TRACE statement.  Maximize the amount of available information!

But in the case of a client/server system, it is a bit trickier.  It is not enough to know that the OnSend or OnReceive methods were called; we need to know which application we're talking about!  When debugging a client/server pair under VS.NET 2003 or later, you can run both the client and the server from the same project in the same version of Visual Studio (right-click on the project name, Debug Ø Start New Instance and you have another app running, whose TRACE output will come out to your output window (at least until you do a compile-and-go, which due to a deep and serious bug in VS, breaks the ability to see TRACE output!)  So all the TRACE statements were modified to be of the form

TRACE(_T("%s: ...stuff here...\n"), AfxGetApp()->m_pszAppName, ...optional parameters here...);

This ensures that the source of the output is always identifiable.  In the example below, I have highlighted the client and server in different colors to make it a little easier to see.  But note that without the identifications of the process name, it would be hard to tell who was sending and who was receiving.

In the protocol shown below, the client and server are started, the client connects to the server, sends the packet "abc", and disconnects.

(Skip around example)

go back to start of example

The Rewritten App

• Project builds (32-bit, 64-bit, ANSI, Unicode)
• Build Directories
• Code Details

On the left, the client allows specifying a dotted quad or a host name as the name for the server to connect to.  I chose "localhost" but I could also have specifed "127.0.0.1".  I left the server port unchanged from the default, 49153 (0xC001), and connected to it.  The connection is asynchronous.  I used the Character Map utility to create a string with some characters in it that are either in the range 128..255 (U0080..U00FF) or some randomly-selected larger Unicode characters.  These applications, as shown in the About box on the right, are the Unicode versions, 32-bit, Debug configuration.  The server used the default port number, and I clicked the Run button to start it before the client.  We see the trace of the socket attachment and then a messge indicating that the transmission was from host 127.0.0.1 via port 1614.  The [10] indicates 10 bytes were received, which is the encoding of the four Unicode characters.  The bytes received, as encoded in UTF-8, are as shown below.  The UTF-8 encoding and decoding is described in a separate essay on my Web site.

Note that the dialogs are resizable and can be stretched so that all values are shown.  I did not deal with the issues of putting  horizontal scrollbar on the ListBox; the details of that are given in an accompanying essay.

Project Builds

This project builds under both VS6 and VS 2005.  Under VS2005 it can build as a 64-bit application:

I would have built a VS.NET 2003 project, but Microsoft has made this impossible without a lot of effort, which I was not inclined to expend in this case.  It is not possible to create a project that can build under VS.NET 2003 and VS2005.

Since VS2005 is installed on my 64-bit machine and I wanted to create a 64-bit app, I didn't have a choice.

The conversion from VS6 to VS2005 was very clumsy because the automated aspects of the conversion are full of bugs, so it took a lot of hand-editing of the project (and finally hand-editing the .vcproj file, which was faster) to get it to convert to what I wanted.  Otherwise, the VS2005 project overwrote the VS6 project.

The following configurations are supported in this project:

Note that each configuration has both a client and a server component, so the VS2005 "batch build" can build 16 executables if all options are selected.

Build directories

Code Details

• Navigating the examples

• Class Summary

• Threading (part 1)



• The listener creates a thread

• CSocketThread::InitInstance

• CSocketThread::ExitInstance

• CSocketThread Message Map

• CSocketThread::OnTermThread

• CSocketThread::OnSendData

• The Client/Server Socket

 

• CConnectSoc::DoClose

• CConnectSoc::OnClose

• Receiving data

• CConnectSoc::OnReceive

• Non-binary expressions of length

• CConnectSoc::NotifyOwnerAboutPacket

• CConnectSoc::SendDataToOwner

• CConnectSoc::SendDebugInfoToOwner

• CConnectSoc::SetDataState

• CConnectSoc::AbortConnection

• Threading (part 2)

• The server application

• CAsyncServerDlg::OnThreadStart

• Performing a shutdown

• CAsyncServerDlg::OnClose

• CAsyncServerDlg::CleanupThreads

• CAsyncServerDlg::OnThreadClose

• CServerSocket::ProcessReceive

• Sending Data

 

• Lock-free queueing

• The Send logic

• CClientThread::Send

• CConnectSoc::Send

• CClientThread::OnSendData

• CConnectSoc::OnSendData

• CConnectSoc::StartNextPacket

• CConnectSoc::OnSend

• CConnectSoc:DoAsyncSendBuff

• CConnectSoc::DoSendComplete

• The Programmer's Interface

• Client Side

• Creating the thread

• CAsyncClientDlg::OnConnectionMade

• Send packets to the thread

• CAsyncClientDlg::OnNetworkData

• CAsyncClientDlg::OnSendComplete

• CAsyncClientDlg::OnNetworkError

• CAsyncClientDlg::OnClose

• CAsyncClientDlg::OnConnectionClose

• CClientSocket::ProcessReceive

• Control Management

• CAsyncClientDlg::updateControls

• Server Side

Navigating the examples

The examples and their explanations are hyperlinked.  Each line that has an explanation has a hyperlink at the end of the line, of the form

     Here there is a line of code // [nn]

Clicking on the hyperlink will go down to the explanation:

Clicking in the hyperlink at the left of the explanation will return to the line of code that the explanation is associated with.

Class Summary

• class CConnectSoc

• class CClientSocket

• class CServerSocket

• class CListenSoc

• class CSocketThread

• class CServerThread

• class CClientThread

• class CAsyncServerDlg

• class CAsyncClientDlg

All socket classes are derived by one or more levels from CAsyncSocket. 

The CConnectSoc class implements a set of operations that handle the message level, that is, splitting the TCP/IP stream into counted message blocks.  This layer also contains utility functions that will transmit either a debug display or the actual string to the owner window, and defines a virtual method that processes the received text in the context of the receiving thread.

The CListenSoc class implements a listener, and its job is to handle the OnAccept notifications and create threads that use a CServerSocket to communicate to the clients.

The CClientSocket and CServerSocket classes implement the client side and server side of the communication.  Essentially, all they provide is an implementation of the virtual method that handles the thread-local message processing.  For this demonstration project, these functions merely invoke the utility functions that transmit the data to the main GUI thread, but in a real system they would be doing substantial work, and only sending notifications to the main GUI thread to display the results of those computations.

class CConnectSoc : public CAsyncSocket {
    public:
        void SetTarget(CWnd * w) { target = w; }
        CWnd * GetTarget() { return target; }

        void SetInfoRequest(BOOL b) { WantPeerInfo = b; }
        BOOL GetInfoRequest() { return WantPeerInfo; }
        BOOL Send(CByteArray & data, DWORD threadid);

    protected:
        CByteArray m_SendBuff;
        INT_PTR bytesSent;
        BOOL Sending;
        CList<CByteArray *, CByteArray *> Queue;

        CByteArray m_recvBuff;

        typedef enum {L0, L1, L2, L3, DATA, ERRORSTATE} ReceiveState; 
        ReceiveState state; 
        union {
           int ByteCount;
           BYTE data[4];
        } Header;       
        void AbortConnection();
        void SetDataState();
        void NotifyOwnerAboutPacket();
        void SendDataToOwner(CByteArray & data);
        void SendDebugInfoToOwner(CByteArray & data);

        virtual void ProcessReceive(CByteArray & data) PURE; // define in subclasses

        virtual void OnConnect(int nErrorCode);
        virtual void OnClose(int nErrorCode);
        virtual void OnReceive(int nErrorCode);
        virtual void OnSend(int nErrorCode);
        ...other members not discussed in this essay
};

class CClientSocket : public CConnectSoc
{
protected:
        virtual void ProcessReceive(CByteArray & data);
        ...other members not discussed in this essay
};

class CServerSocket : public CConnectSoc
{
protected:
        virtual void ProcessReceive(CByteArray & data);
        ...other members not discussed in this essay
};

class CListensoc : public CAsyncSocket
{
 public:
        void SetTarget(CWnd * w) { target = w; }
 protected:
        virtual void OnAccept(int nErrorCode);

        CDWordArray m_threadIDs;
        CWnd * target;
        ...other members not discussed in this essay
};

class CSocketThread : public CWinThread {
    protected:
       DECLARE_DYNCREATE(CSocketThread)
    public:
       CSocketThread();
       void Send(CByteArray & data) { CConnectSoc * sock = GetSocket();
                                      ASSERT(sock != NULL);
                                      if(sock != NULL)
                                         sock->Send(data, m_nThreadID);
                                    }
       void SetSocket(SOCKET h) { socket = h; }
       void SetTarget(CWnd * w) { target = w; }
    protected:
       virtual CConnectSoc * GetSocket() {ASSERT(FALSE); return NULL; } // implement in subclasses
                       // for example in CServerThread::GetSocket or CClientThread::GetSocket

       // Used to pass the socket handle from the main thread to this thread.
       SOCKET socket;
       // Target of messages which are posted
       CWnd * target;
       virtual BOOL InitInstance();
       virtual int ExitInstance();
    protected:
       virtual ~CSocketThread();
       afx_msg void OnTermThread(WPARAM, LPARAM);
       afx_msg void OnSendData(WPARAM, LPARAM);
       afx_msg void OnStartNextPacket(WPARAM, LPARAM);
       DECLARE_MESSAGE_MAP()
};

class CServerThread : public CSocketThread
{
protected:
        CServerSocket m_socket;
        virtual CConnectSoc * GetSocket() { return &m_socket; }

        virtual BOOL InitInstance();
        virtual int ExitInstance();

        ...other members not discussed in this essay
};

class CClientThread : public CSocketThread
{
public:
        void SetServerName(const CString & name) { m_ServerName = name; }
        void SetPort(UINT p) { m_port = p; }
protected:
        virtual BOOL InitInstance();
        virtual int ExitInstance();
protected:
        CClientSocket m_socket;
        virtual CConnectSoc * GetSocket() { return &m_socket; }

        CString m_ServerName;

        UINT m_port;
        
        ...other members not discussed in this essay
};

class CAsyncServerDlg : public CDialog {
protected:
        CDWordArray m_threadIDs;
        BOOL m_MainWndIsClosing; 
        BOOL CleanupThreads();
        afx_msg void OnClose();
        ...other members not discussed in this essay
};

class CAsyncClientDlg : public CDialog
{
// Construction
    public:
       CAsyncClientDlg(CWnd* pParent = NULL);  // standard constructor

    protected:// Dialog Data
       CClientThread* m_pClientThread;
       CString m_ServerName;
       BOOL m_Connected;
       BOOL m_Connecting;
       BOOL m_MainWndIsClosing;
       BOOL m_Sending;
       
protected:
        CFont font;
        BOOL CleanupThread();
        
        void updateControls();

        LRESULT OnInfo(WPARAM, LPARAM);
        LRESULT OnConnectionClose(WPARAM, LPARAM);   // a connection has been closed
        LRESULT OnNetworkData(WPARAM, LPARAM);       // received new message
        LRESULT OnConnectionMade(WPARAM, LPARAM);    // pending connection has been established
        LRESULT OnNetworkError(WPARAM, LPARAM);      // a network error occurred
        ...other members not discussed in this essay
};

Threading (part 1) (part 2)

Since the key defect in the original code was its completely erroneous handling of threading, I'm going to discuss in detail what I did here.

The thread is going to require several key pieces of information.  It will need the IP address of the target (or its DNS name), the desired remote port to connect to, and the window to which notifications should be posted.  Rather than set all kinds of variables including members-of-members, a triple of Set methods of the thread are provided

void CClientThread::SetTarget(CWnd *);
void CClientThread::SetServerName(const CString &);
void CClientThread::SetPort(UINT);

Unlike the previous example, there is no need to set thread pointers or other incidentals, since they serve no useful purpose.  In the spirit of not binding the port number at compile time, the port number is now a user-definable option at run time.

The listener creates a thread

The thread creation code is now as shown below.  This code is  called when the CAsyncSocket class gets a notification that an Accept will not block.

void CListensoc::OnAccept(int nErrorCode) 
   {
    TRACE(_T("%s: CListensoc::OnAccept(%d)\n"), AfxGetApp()->m_pszAppName, nErrorCode);                // [1]
    if(nErrorCode != ERROR_SUCCESS)                                     // [2]
       { /* had an error */
        ASSERT(FALSE);                                                  // [3]
        ASSERT(target != NULL);                                         // [4]
        if(target != NULL)                                              // [5]
           target->PostMessage(UWM_NETWORK_ERROR, (WPARAM)nErrorCode, (LPARAM)::GetCurrentThreadId());  // [6]
        return;
       } /* had an error */
    // New connection is being established

    CAsyncSocket soc;

    // Accept the connection using a temp CAsyncSocket object.
    if(!Accept(soc))                                                    // [7]
       { /* accept failed */
        DWORD err = ::GetLastError();                                   // [8]
        ASSERT(FALSE);                                                  // [9]
        ASSERT(target != NULL);
        if(target != NULL)
           target->PostMessage(UWM_NETWORK_ERROR, (WPARAM)err, (LPARAM)::GetCurrentThreadId());
        return;
       } /* accept failed */

    // Create a thread to handle the connection. The thread is created suspended so that we can
    // set variables in CServerThread before it starts executing.
    CServerThread * pThread = (CServerThread *)AfxBeginThread(RUNTIME_CLASS(CServerThread),
                                                            THREAD_PRIORITY_NORMAL,
                                                            0,
                                                            CREATE_SUSPENDED);  // [10]
    if (pThread == NULL)
        {
         DWORD err = ::GetLastError();                                  // [11]
         ASSERT(FALSE);  // help in debugging                           // [12]
         soc.Close();                                                   // [13]
         TRACE(_T("%s: CListenSoc::OnAccept: Could not create thread: %s\n"), 
                     AfxGetApp()->m_pszAppName, ErrorString(err));      // [14]
         ASSERT(target != NULL);
         if(target != NULL)
            target->PostMessage(UWM_NETWORK_ERROR, (WPARAM)err, (LPARAM)::GetCurrentThreadId());  // [15]
         return;
        }

    // Pass the socket to the thread by passing the socket handle. You cannot pass
    // a CSocket object across threads.
    pThread->SetSocket(soc.Detach());                                   // [16]
    pThread->SetTarget(target);                                         // [17]

    // Now start the thread.
    pThread->ResumeThread();                                            // [18]
        
    CAsyncSocket::OnAccept(nErrorCode);
   }

CAsyncSocket::~CAsyncSocket()
{
	if (m_hSocket != INVALID_SOCKET)
		Close();
}