学步园

一位朋友刚开始接触SOCKET编程，给了一段代码请我帮忙看看，前面两个函数如下（注释是我加的）：

void CtransferDlg::OnBnClickedBtnStart() {
    SOCKET sockSrv = socket(AF_INET, SOCK_STREAM, 0);
    SOCKADDR_IN addrSrv;
    addrSrv.sin_addr.S_un.S_addr = htonl(INADDR_ANY); //这里似乎应该用ntohl()
    addrSrv.sin_family = AF_INET;
    addrSrv.sin_port = htons(7001);  //同理，这里似乎应该用ntohs()
    if (SOCKET_ERROR == bind(sockSrv ,(SOCKADDR*)&addrSrv , sizeof(SOCKADDR))) {
        MessageBox("地址邦定失败!");
        return; //这里直接返回了，sockSrv这个资源未得到释放
    }
    if (SOCKET_ERROR == listen(sockSrv , 5)) {
        MessageBox("监听失败!");
        return; //同样，如果启动侦听失败，sockSrv这个资源得不到释放
    }
    ::CreateThread(NULL, 0, listenThread, (LPVOID)sockSrv, 0, NULL);
//未判断CreateThread的返回值，不知道线程是否创建成功。
//未保存返回的线程句柄并且立刻CloseHandle这个句柄（问题不大，但这是细节）
//本函数退出了，sockSrv这个资源似乎只有让新创建的线程来负责释放，否则……
}

//下面是负责Accept的线程（所以listenThread这个函数名似乎取得有点问题）
DWORD WINAPI CtransferDlg::listenThread(LPVOID lpParameter) {
    SOCKADDR_IN addrClient ;
    int len = sizeof( SOCKADDR_IN ); //这句应该搬到accept(...)这一句的前面。
//注意下面开始了一个无限循环，没有任何出口
    while(TRUE) {
        SOCKET sockConn = accept((SOCKET)lpParameter, (SOCKADDR *)&addrClient , &len);
//忘了判断返回值，accept也会失败（比如sockSrv被另一个线程关闭）
        char recvBuf[100]; //注意这个缓冲区在栈上，并且由多个客户端连接共享
        recv(sockConn , recvBuf , 100 , 0); //这里100应该用sizeof(recvBuf)来代替，不应该用硬编码
//同样也忘了判断recv()的返回值，而且，还错误地用了阻塞式recv
        closesocket(sockConn); //不知道为什么要立刻断开客户端
        ::CreateThread(NULL, 0, ConnectThread, (LPVOID)recvBuf, 0, NULL);
//创建一个新线程来处理数据，但recvBuf也许会被下一个进来的客户端发来的数据覆盖掉
        //每一个客户端连接建立一个线程来处理，也不是一个好办法
    }
    //这个循环永远退不出来，而且传进来的lpParameter其实是sockSrv，
    //没有任何地方会释放它（closesocket）
    return 0;
}

上面两个函数，问题不少，简单归纳一下，主要是：

• 对资源的释放问题漫不经心
  资源不用了，应该立刻释放，虽然在进程终止以后，操作系统会回收所有资源，但一个程序（特别是7×24运行的服务端程序）应该做到不出任何一点点泄露。
• 对多线程理解不深
  多线程编程是个很复杂的逻辑（在对共享资源进行访问时会遇到各种各样稀奇古怪的问题），而且对程序性能影响很大（比方说，如果按上面的代码为每一个客户端都创建一个线程，如果这些线程都不休眠的话，那CPU被拖得100%占用率很容易）。
• 缺乏判断函数返回值的习惯
  正确的习惯应该是判断每一个具有返回值的函数（如果函数总是成功，其返回值肯定是void型的）。

今天有点晚了，明天再来写通常的做法应该是怎样的。